Skip to content

Commit 2385b5c

Browse files
Fix usage of Resolve-RepositoryElements -DisableValidation (#243)
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.
1 parent 06f596e commit 2385b5c

7 files changed

+73
-18
lines changed

Diff for: GitHubContents.ps1

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@
138138

139139
Write-InvocationLog
140140

141-
$elements = Resolve-RepositoryElements -DisableValidation
141+
$elements = Resolve-RepositoryElements
142142
$OwnerName = $elements.ownerName
143143
$RepositoryName = $elements.repositoryName
144144

Diff for: GitHubIssues.ps1

+4
Original file line numberDiff line numberDiff line change
@@ -219,13 +219,17 @@ filter Get-GitHubIssue
219219

220220
Write-InvocationLog
221221

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

226229
$telemetryProperties = @{
227230
'OwnerName' = (Get-PiiSafeString -PlainText $OwnerName)
228231
'RepositoryName' = (Get-PiiSafeString -PlainText $RepositoryName)
232+
'OrganizationName' = (Get-PiiSafeString -PlainText $OrganizationName)
229233
'ProvidedIssue' = $PSBoundParameters.ContainsKey('Issue')
230234
}
231235

Diff for: GitHubMiscellaneous.ps1

+22-2
Original file line numberDiff line numberDiff line change
@@ -310,14 +310,18 @@ filter Get-GitHubLicense
310310

311311
Write-InvocationLog
312312

313+
$telemetryProperties = @{}
314+
315+
# Intentionally disabling validation because parameter sets exist that both require
316+
# OwnerName/RepositoryName (to get that repo's License) as well as don't (to get
317+
# all known Licenses). We'll do additional parameter validation within the function.
313318
$elements = Resolve-RepositoryElements -DisableValidation
314319
$OwnerName = $elements.ownerName
315320
$RepositoryName = $elements.repositoryName
316321

317-
$telemetryProperties = @{}
318-
319322
$uriFragment = 'licenses'
320323
$description = 'Getting all licenses'
324+
321325
if ($PSBoundParameters.ContainsKey('Key'))
322326
{
323327
$telemetryProperties['Key'] = $Name
@@ -331,6 +335,12 @@ filter Get-GitHubLicense
331335
$uriFragment = "repos/$OwnerName/$RepositoryName/license"
332336
$description = "Getting the license for $RepositoryName"
333337
}
338+
elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName))
339+
{
340+
$message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.'
341+
Write-Log -Message $message -Level Error
342+
throw $message
343+
}
334344

335345
$params = @{
336346
'UriFragment' = $uriFragment
@@ -537,6 +547,9 @@ filter Get-GitHubCodeOfConduct
537547

538548
Write-InvocationLog
539549

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

546559
$uriFragment = 'codes_of_conduct'
547560
$description = 'Getting all Codes of Conduct'
561+
548562
if ($PSBoundParameters.ContainsKey('Key'))
549563
{
550564
$telemetryProperties['Key'] = $Name
@@ -558,6 +572,12 @@ filter Get-GitHubCodeOfConduct
558572
$uriFragment = "repos/$OwnerName/$RepositoryName/community/code_of_conduct"
559573
$description = "Getting the Code of Conduct for $RepositoryName"
560574
}
575+
elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName))
576+
{
577+
$message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.'
578+
Write-Log -Message $message -Level Error
579+
throw $message
580+
}
561581

562582
$params = @{
563583
'UriFragment' = $uriFragment

Diff for: GitHubReleases.ps1

+1-1
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ filter Get-GitHubRelease
176176

177177
Write-InvocationLog -Invocation $MyInvocation
178178

179-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters -DisableValidation
179+
$elements = Resolve-RepositoryElements
180180
$OwnerName = $elements.ownerName
181181
$RepositoryName = $elements.repositoryName
182182

Diff for: GitHubRepositories.ps1

+23-12
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ filter Remove-GitHubRepository
306306
DefaultParameterSetName='Elements',
307307
ConfirmImpact="High")]
308308
[Alias('Delete-GitHubRepository')]
309+
[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.")]
309310
param(
310311
[Parameter(ParameterSetName='Elements')]
311312
[string] $OwnerName,
@@ -329,7 +330,7 @@ filter Remove-GitHubRepository
329330

330331
Write-InvocationLog -Invocation $MyInvocation
331332

332-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
333+
$elements = Resolve-RepositoryElements
333334
$OwnerName = $elements.ownerName
334335
$RepositoryName = $elements.repositoryName
335336

@@ -548,7 +549,11 @@ filter Get-GitHubRepository
548549

549550
Write-InvocationLog -Invocation $MyInvocation
550551

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

@@ -1012,7 +1017,7 @@ filter Update-GitHubRepository
10121017

10131018
Write-InvocationLog -Invocation $MyInvocation
10141019

1015-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1020+
$elements = Resolve-RepositoryElements
10161021
$OwnerName = $elements.ownerName
10171022
$RepositoryName = $elements.repositoryName
10181023

@@ -1131,6 +1136,7 @@ filter Get-GitHubRepositoryTopic
11311136
DefaultParameterSetName='Elements')]
11321137
[OutputType({$script:GitHubRepositoryTopicTypeName})]
11331138
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1139+
[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.")]
11341140
param(
11351141
[Parameter(ParameterSetName='Elements')]
11361142
[string] $OwnerName,
@@ -1152,7 +1158,7 @@ filter Get-GitHubRepositoryTopic
11521158

11531159
Write-InvocationLog -Invocation $MyInvocation
11541160

1155-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1161+
$elements = Resolve-RepositoryElements
11561162
$OwnerName = $elements.ownerName
11571163
$RepositoryName = $elements.repositoryName
11581164

@@ -1316,7 +1322,7 @@ function Set-GitHubRepositoryTopic
13161322
{
13171323
Write-InvocationLog -Invocation $MyInvocation
13181324

1319-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1325+
$elements = Resolve-RepositoryElements
13201326
$OwnerName = $elements.ownerName
13211327
$RepositoryName = $elements.repositoryName
13221328

@@ -1434,6 +1440,7 @@ filter Get-GitHubRepositoryContributor
14341440
[OutputType({$script:GitHubUserTypeName})]
14351441
[OutputType({$script:GitHubRepositoryContributorStatisticsTypeName})]
14361442
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1443+
[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.")]
14371444
param(
14381445
[Parameter(ParameterSetName='Elements')]
14391446
[string] $OwnerName,
@@ -1459,7 +1466,7 @@ filter Get-GitHubRepositoryContributor
14591466

14601467
Write-InvocationLog -Invocation $MyInvocation
14611468

1462-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1469+
$elements = Resolve-RepositoryElements
14631470
$OwnerName = $elements.ownerName
14641471
$RepositoryName = $elements.repositoryName
14651472

@@ -1577,8 +1584,9 @@ filter Get-GitHubRepositoryCollaborator
15771584
[CmdletBinding(
15781585
SupportsShouldProcess,
15791586
DefaultParameterSetName='Elements')]
1580-
[OutputType({$script:GitHubUserTypeName})]
1581-
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1587+
[OutputType({$script:GitHubUserTypeName})]
1588+
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1589+
[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.")]
15821590
param(
15831591
[Parameter(ParameterSetName='Elements')]
15841592
[string] $OwnerName,
@@ -1603,7 +1611,7 @@ filter Get-GitHubRepositoryCollaborator
16031611

16041612
Write-InvocationLog -Invocation $MyInvocation
16051613

1606-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1614+
$elements = Resolve-RepositoryElements
16071615
$OwnerName = $elements.ownerName
16081616
$RepositoryName = $elements.repositoryName
16091617

@@ -1692,6 +1700,7 @@ filter Get-GitHubRepositoryLanguage
16921700
DefaultParameterSetName='Elements')]
16931701
[OutputType({$script:GitHubRepositoryLanguageTypeName})]
16941702
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1703+
[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.")]
16951704
param(
16961705
[Parameter(ParameterSetName='Elements')]
16971706
[string] $OwnerName,
@@ -1713,7 +1722,7 @@ filter Get-GitHubRepositoryLanguage
17131722

17141723
Write-InvocationLog -Invocation $MyInvocation
17151724

1716-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1725+
$elements = Resolve-RepositoryElements
17171726
$OwnerName = $elements.ownerName
17181727
$RepositoryName = $elements.repositoryName
17191728

@@ -1798,6 +1807,7 @@ filter Get-GitHubRepositoryTag
17981807
DefaultParameterSetName='Elements')]
17991808
[OutputType({$script:GitHubRepositoryTagTypeName})]
18001809
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1810+
[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.")]
18011811
param(
18021812
[Parameter(ParameterSetName='Elements')]
18031813
[string] $OwnerName,
@@ -1819,7 +1829,7 @@ filter Get-GitHubRepositoryTag
18191829

18201830
Write-InvocationLog -Invocation $MyInvocation
18211831

1822-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1832+
$elements = Resolve-RepositoryElements
18231833
$OwnerName = $elements.ownerName
18241834
$RepositoryName = $elements.repositoryName
18251835

@@ -1909,6 +1919,7 @@ filter Move-GitHubRepositoryOwnership
19091919
[OutputType({$script:GitHubRepositoryTypeName})]
19101920
[Alias('Transfer-GitHubRepositoryOwnership')]
19111921
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")]
1922+
[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.")]
19121923
param(
19131924
[Parameter(ParameterSetName='Elements')]
19141925
[string] $OwnerName,
@@ -1936,7 +1947,7 @@ filter Move-GitHubRepositoryOwnership
19361947

19371948
Write-InvocationLog -Invocation $MyInvocation
19381949

1939-
$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
1950+
$elements = Resolve-RepositoryElements
19401951
$OwnerName = $elements.ownerName
19411952
$RepositoryName = $elements.repositoryName
19421953

Diff for: GitHubRepositoryForks.ps1

+2-2
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ filter Get-GitHubRepositoryFork
9191

9292
Write-InvocationLog
9393

94-
$elements = Resolve-RepositoryElements -DisableValidation
94+
$elements = Resolve-RepositoryElements
9595
$OwnerName = $elements.ownerName
9696
$RepositoryName = $elements.repositoryName
9797

@@ -211,7 +211,7 @@ filter New-GitHubRepositoryFork
211211

212212
Write-InvocationLog
213213

214-
$elements = Resolve-RepositoryElements -DisableValidation
214+
$elements = Resolve-RepositoryElements
215215
$OwnerName = $elements.ownerName
216216
$RepositoryName = $elements.repositoryName
217217

Diff for: Tests/GitHubMiscellaneous.tests.ps1

+20
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ Describe 'Get-GitHubLicense' {
9999
}
100100
}
101101

102+
Context 'Will fail if not provided both OwnerName and RepositoryName' {
103+
It 'Should fail if only OwnerName is specified' {
104+
{ Get-GitHubLicense -OwnerName 'PowerShell' } | Should -Throw
105+
}
106+
107+
It 'Should fail if only RepositoryName is specified' {
108+
{ Get-GitHubLicense -RepositoryName 'PowerShell' } | Should -Throw
109+
}
110+
}
111+
102112
Context 'Can get the license for a repo with the repo on the pipeline' {
103113
BeforeAll {
104114
$result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubLicense
@@ -213,6 +223,16 @@ Describe 'Get-GitHubCodeOfConduct' {
213223
}
214224
}
215225

226+
Context 'Will fail if not provided both OwnerName and RepositoryName' {
227+
It 'Should fail if only OwnerName is specified' {
228+
{ Get-GitHubCodeOfConduct -OwnerName 'PowerShell' } | Should -Throw
229+
}
230+
231+
It 'Should fail if only RepositoryName is specified' {
232+
{ Get-GitHubCodeOfConduct -RepositoryName 'PowerShell' } | Should -Throw
233+
}
234+
}
235+
216236
Context 'Can get the code of conduct for a repo with the repo on the pipeline' {
217237
BeforeAll {
218238
$result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubCodeOfConduct

0 commit comments

Comments
 (0)