Skip to content

Commit

Permalink
Fix usage of Resolve-RepositoryElements -DisableValidation (#243)
Browse files Browse the repository at this point in the history
There are only four legitimate instances in the module as of today where we should be disabling the validation that Resolve-RepositoryElements provides.  All other instances were likely introduced due to mistaken copy/paste issues.

The only reason we'd want to disable the validation is if there are multiple parametersets that need to be considered for the function, where at least one _would_ require `OwnerName`/`RepositoryName` while at least one other one _would not_.

This would have meant that we would have attempted to call those GitHub endpoints with incomplete path information which would have most likely resulted in an error that we could have caught sooner without needing to invoke a web request.

I've fixed up all of these incorrect instances, and added a comment to the two remaining ones to remind others later on why that switch is being used, and help prevent incorrect usage in the future.

I've also cleaned up unnecessary passing of `BoundParameters` since it already does the correct thing by default.
  • Loading branch information
HowardWolosky committed Jun 21, 2020
1 parent 06f596e commit 2385b5c
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 18 deletions.
2 changes: 1 addition & 1 deletion GitHubContents.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@

Write-InvocationLog

$elements = Resolve-RepositoryElements -DisableValidation
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down
4 changes: 4 additions & 0 deletions GitHubIssues.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,17 @@ filter Get-GitHubIssue

Write-InvocationLog

# Intentionally disabling validation here because parameter sets exist that do not require
# an OwnerName and RepositoryName. Therefore, we will do futher parameter validation further
# into the function.
$elements = Resolve-RepositoryElements -DisableValidation
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

$telemetryProperties = @{
'OwnerName' = (Get-PiiSafeString -PlainText $OwnerName)
'RepositoryName' = (Get-PiiSafeString -PlainText $RepositoryName)
'OrganizationName' = (Get-PiiSafeString -PlainText $OrganizationName)
'ProvidedIssue' = $PSBoundParameters.ContainsKey('Issue')
}

Expand Down
24 changes: 22 additions & 2 deletions GitHubMiscellaneous.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -310,14 +310,18 @@ filter Get-GitHubLicense

Write-InvocationLog

$telemetryProperties = @{}

# Intentionally disabling validation because parameter sets exist that both require
# OwnerName/RepositoryName (to get that repo's License) as well as don't (to get
# all known Licenses). We'll do additional parameter validation within the function.
$elements = Resolve-RepositoryElements -DisableValidation
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

$telemetryProperties = @{}

$uriFragment = 'licenses'
$description = 'Getting all licenses'

if ($PSBoundParameters.ContainsKey('Key'))
{
$telemetryProperties['Key'] = $Name
Expand All @@ -331,6 +335,12 @@ filter Get-GitHubLicense
$uriFragment = "repos/$OwnerName/$RepositoryName/license"
$description = "Getting the license for $RepositoryName"
}
elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName))
{
$message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.'
Write-Log -Message $message -Level Error
throw $message
}

$params = @{
'UriFragment' = $uriFragment
Expand Down Expand Up @@ -537,6 +547,9 @@ filter Get-GitHubCodeOfConduct

Write-InvocationLog

# Intentionally disabling validation because parameter sets exist that both require
# OwnerName/RepositoryName (to get that repo's Code of Conduct) as well as don't (to get
# all known Codes of Conduct). We'll do additional parameter validation within the function.
$elements = Resolve-RepositoryElements -DisableValidation
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName
Expand All @@ -545,6 +558,7 @@ filter Get-GitHubCodeOfConduct

$uriFragment = 'codes_of_conduct'
$description = 'Getting all Codes of Conduct'

if ($PSBoundParameters.ContainsKey('Key'))
{
$telemetryProperties['Key'] = $Name
Expand All @@ -558,6 +572,12 @@ filter Get-GitHubCodeOfConduct
$uriFragment = "repos/$OwnerName/$RepositoryName/community/code_of_conduct"
$description = "Getting the Code of Conduct for $RepositoryName"
}
elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName))
{
$message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.'
Write-Log -Message $message -Level Error
throw $message
}

$params = @{
'UriFragment' = $uriFragment
Expand Down
2 changes: 1 addition & 1 deletion GitHubReleases.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ filter Get-GitHubRelease

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters -DisableValidation
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down
35 changes: 23 additions & 12 deletions GitHubRepositories.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ filter Remove-GitHubRepository
DefaultParameterSetName='Elements',
ConfirmImpact="High")]
[Alias('Delete-GitHubRepository')]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -329,7 +330,7 @@ filter Remove-GitHubRepository

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -548,7 +549,11 @@ filter Get-GitHubRepository

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters -DisableValidation
# We are explicitly disabling validation here because a valid parameter set for this function
# allows the OwnerName to be passed in, but not the RepositoryName. That would allow the caller
# to get all of the repositories owned by a specific username. Therefore, we don't want to fail
# if both have not been supplied...we'll do the extra validation within the function.
$elements = Resolve-RepositoryElements -DisableValidation
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1012,7 +1017,7 @@ filter Update-GitHubRepository

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1131,6 +1136,7 @@ filter Get-GitHubRepositoryTopic
DefaultParameterSetName='Elements')]
[OutputType({$script:GitHubRepositoryTopicTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -1152,7 +1158,7 @@ filter Get-GitHubRepositoryTopic

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1316,7 +1322,7 @@ function Set-GitHubRepositoryTopic
{
Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1434,6 +1440,7 @@ filter Get-GitHubRepositoryContributor
[OutputType({$script:GitHubUserTypeName})]
[OutputType({$script:GitHubRepositoryContributorStatisticsTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -1459,7 +1466,7 @@ filter Get-GitHubRepositoryContributor

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1577,8 +1584,9 @@ filter Get-GitHubRepositoryCollaborator
[CmdletBinding(
SupportsShouldProcess,
DefaultParameterSetName='Elements')]
[OutputType({$script:GitHubUserTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[OutputType({$script:GitHubUserTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -1603,7 +1611,7 @@ filter Get-GitHubRepositoryCollaborator

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1692,6 +1700,7 @@ filter Get-GitHubRepositoryLanguage
DefaultParameterSetName='Elements')]
[OutputType({$script:GitHubRepositoryLanguageTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -1713,7 +1722,7 @@ filter Get-GitHubRepositoryLanguage

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1798,6 +1807,7 @@ filter Get-GitHubRepositoryTag
DefaultParameterSetName='Elements')]
[OutputType({$script:GitHubRepositoryTagTypeName})]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand All @@ -1819,7 +1829,7 @@ filter Get-GitHubRepositoryTag

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -1909,6 +1919,7 @@ filter Move-GitHubRepositoryOwnership
[OutputType({$script:GitHubRepositoryTypeName})]
[Alias('Transfer-GitHubRepositoryOwnership')]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")]
param(
[Parameter(ParameterSetName='Elements')]
[string] $OwnerName,
Expand Down Expand Up @@ -1936,7 +1947,7 @@ filter Move-GitHubRepositoryOwnership

Write-InvocationLog -Invocation $MyInvocation

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down
4 changes: 2 additions & 2 deletions GitHubRepositoryForks.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ filter Get-GitHubRepositoryFork

Write-InvocationLog

$elements = Resolve-RepositoryElements -DisableValidation
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down Expand Up @@ -211,7 +211,7 @@ filter New-GitHubRepositoryFork

Write-InvocationLog

$elements = Resolve-RepositoryElements -DisableValidation
$elements = Resolve-RepositoryElements
$OwnerName = $elements.ownerName
$RepositoryName = $elements.repositoryName

Expand Down
20 changes: 20 additions & 0 deletions Tests/GitHubMiscellaneous.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,16 @@ Describe 'Get-GitHubLicense' {
}
}

Context 'Will fail if not provided both OwnerName and RepositoryName' {
It 'Should fail if only OwnerName is specified' {
{ Get-GitHubLicense -OwnerName 'PowerShell' } | Should -Throw
}

It 'Should fail if only RepositoryName is specified' {
{ Get-GitHubLicense -RepositoryName 'PowerShell' } | Should -Throw
}
}

Context 'Can get the license for a repo with the repo on the pipeline' {
BeforeAll {
$result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubLicense
Expand Down Expand Up @@ -213,6 +223,16 @@ Describe 'Get-GitHubCodeOfConduct' {
}
}

Context 'Will fail if not provided both OwnerName and RepositoryName' {
It 'Should fail if only OwnerName is specified' {
{ Get-GitHubCodeOfConduct -OwnerName 'PowerShell' } | Should -Throw
}

It 'Should fail if only RepositoryName is specified' {
{ Get-GitHubCodeOfConduct -RepositoryName 'PowerShell' } | Should -Throw
}
}

Context 'Can get the code of conduct for a repo with the repo on the pipeline' {
BeforeAll {
$result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubCodeOfConduct
Expand Down

0 comments on commit 2385b5c

Please sign in to comment.