Skip to content
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.

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

Use module-qualified cmdlet names when invoking plugins #129

Merged
merged 2 commits into from
Oct 31, 2018

Conversation

Tadas
Copy link
Contributor

@Tadas Tadas commented Oct 24, 2018

Description

I wanted to use start cmdlet name in a plugin but discovered that command execution jobs never finish executing. Turns out that start was being resolved as Start-Process by PowerShell. PoshBot should use module-qualified cmdlet names when invoking plugin commands to ensure that the right cmdlet is called regardless of aliases.

https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_command_precedence?view=powershell-6#qualified-names

Related Issue

Motivation and Context

Enables usage of cmdlet names in plugins which might be hidden by an alias e.g. start (a default alias for Start-Process)

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@devblackops
Copy link
Member

Good idea @Tadas. Thanks!

@devblackops
Copy link
Member

@Tadas I'm trying to repro your issue but can't get the problem to occur. This is what I'm trying:

function Start {
    [cmdletbinding()]
    param()

    'Start your engines'
}

or

function Start-Engine {
    [PoshBot.BotCommand(
        CommandName = 'start'
    )]
    [cmdletbinding()]
    param()

    'Start your engines'
}

or

function Start-Engine {
    [PoshBot.BotCommand(
        Aliases = 'start'
    )]
    [cmdletbinding()]
    param()

    'Start your engines'
}

All of these seem to work fine.

What does your plugin command look like that triggers this?

@Tadas
Copy link
Contributor Author

Tadas commented Oct 26, 2018

It can only happen when you have an alias defined

> Get-Alias -Name start

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Alias           start -> Start-Process


> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.18262.1000
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18262.1000
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

Only your first example triggers the bug, that function is pretty much identical to what I have.

@devblackops
Copy link
Member

Sorry still can't repro this, and I'm not sure why you're getting a problem either. 🤷‍♂️

PoshBot is already executing the actual command from the module, not the module-qualified name, but the actual function/cmdlet object returned from Get-Command.

When it loads the module, it gets all the module's public commands via Get-Command here.
It then stores each object in either the .FunctionInfo or .CmdletInfo property of the [Command] instance depending on if the command is of type Function or Cmdlet here. When it comes time to execute a command in a job, it adds either the .FunctionInfo or .CmdletInfo property to the hashtable that will be sent to the job here. Inside the job, it executes the actual command object here and here.

It never executes the command by name only which I can see would cause a problem with aliases of the same name.

Below is effectively what PoshBot does and even though I've added an alias for start, it still executes the module command because it's not being executed by name.

New-Alias -Name start -Value Start-Process
$c = Get-Command -Module PoshBot.PSSummit -Name start
& $c

I'd like to understand more about how you're running into this.

@Tadas
Copy link
Contributor Author

Tadas commented Oct 27, 2018

Ok I think we're running into serialization nuances. Is your plugin begin executed as job?

Just before Start-Job call:

$jobParams = @{
Name = $jobName
ScriptBlock = $outer
ArgumentList = $options
}
return (Start-Job @jobParams)

[DBG]: PS D:\Dev\_PoshBot>> $jobParams.ArgumentList.Function.GetType()

IsPublic IsSerial Name                                     BaseType                                          
-------- -------- ----                                     --------                                          
True     False    FunctionInfo                             System.Management.Automation.CommandInfo          

and inside of the job:

[DBG]: [Job14]: PS D:\Documents>> l


   26:                  ParsedCommand = $options.ParsedCommand | Select-Object -ExcludeProperty $parsedCommandExcludes
   27:                  OriginalMessage = $options.OriginalMessage
   28:                  BackendType = $options.BackendType
   29:              }
   30:              Wait-Debugger
   31:*             & $cmd @namedParameters @positionalParameters
   32:          



[DBG]: [Job14]: PS D:\Documents>> $cmd.GetType()

IsPublic IsSerial Name                                     BaseType                                          
-------- -------- ----                                     --------                                          
True     True     PSObject                                 System.Object                                     



[DBG]: [Job14]: PS D:\Documents>> $cmd | gm


   TypeName: Deserialized.System.Management.Automation.FunctionInfo

Name                MemberType   Definition                                                                  
----                ----------   ----------                                                                  
GetType             Method       type GetType()                                                              
ToString            Method       string ToString(), string ToString(string format, System.IFormatProvider ...
Namespace           NoteProperty string Namespace=ReproModule                                                
CmdletBinding       Property     System.Boolean {get;set;}                                                   
CommandType         Property     System.String {get;set;}                                                    
DefaultParameterSet Property      {get;set;}                                                                 
Definition          Property     System.String {get;set;}                                                    
Description         Property      {get;set;}                                                                 
HelpFile            Property      {get;set;}                                                                 
Module              Property     System.String {get;set;}                                                    
ModuleName          Property     System.String {get;set;}                                                    
Name                Property     System.String {get;set;}                                                    
Noun                Property     System.String {get;set;}                                                    
Options             Property     System.String {get;set;}                                                    
OutputType          Property     Deserialized.System.Collections.ObjectModel.ReadOnlyCollection`1[[System....
Parameters          Property     Deserialized.System.Collections.Generic.Dictionary`2[[System.String, msco...
ParameterSets       Property     Deserialized.System.Collections.ObjectModel.ReadOnlyCollection`1[[System....
RemotingCapability  Property     System.String {get;set;}                                                    
ScriptBlock         Property     System.String {get;set;}                                                    
Source              Property     System.String {get;set;}                                                    
Verb                Property     System.String {get;set;}                                                    
Version             Property     System.Version {get;set;}                                                   
Visibility          Property     System.String {get;set;}    

@devblackops
Copy link
Member

Ahh. That makes sense.

I'm OK with this change to use the module qualified name.

Copy link
Member

@devblackops devblackops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devblackops devblackops merged commit 03a8e9b into poshbotio:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants