From 6cf344fb38485275f94b1e85c1a5f932e1b519c3 Mon Sep 17 00:00:00 2001 From: Howard Wolosky Date: Fri, 30 Nov 2018 11:26:13 -0800 Subject: [PATCH] Prevent `ConvertTo-SmarterObject` from flattening arrays (#56) * Ensure that `ConvertTo-SmarterObject` does not cause any side-effects on `InputObject` * Ensure that `ConvertTo-SmarterObject` does not flatten any arrays * Switched all calls of `ConvertTo-Json` to not use pipelining in order to avoid PowerShell's array-flattening logic when piping the InputObject in, as opposed to when passing it as a parameter. * Ensured that we do best-effort Date conversion in `ConvertTo-SmarterObject` (a failed date conversion should never cause an exception/failure). * Used a workaround described in [Pester's Wiki](# https://github.com/pester/Pester/wiki/Testing-different-module-types) to force the module to be a `Script Module` instead of a `Manifest Module` so that `Mock` and `InModuleScope` (which lets you test private methods) work. * Added UT's for `ConvertTo-SmarterObject` core scenarios. Resolves PowerShell#55: ConvertTo-SmarterObject is flattening arrays Thanks to @DanBelcher-MSFT for the assist on this one. --- GitHubConfiguration.ps1 | 5 +- GitHubCore.ps1 | 41 ++++-- GitHubIssues.ps1 | 6 +- GitHubLabels.ps1 | 4 +- GitHubMiscellaneous.ps1 | 2 +- GitHubRepositories.ps1 | 8 +- GitHubUsers.ps1 | 2 +- Helpers.ps1 | 2 +- PowerShellForGitHub.psd1 | 2 +- PowerShellForGitHub.psm1 | 4 + Tests/GitHubCore.Tests.ps1 | 267 +++++++++++++++++++++++++++++++++++++ 11 files changed, 315 insertions(+), 28 deletions(-) create mode 100644 PowerShellForGitHub.psm1 create mode 100644 Tests/GitHubCore.Tests.ps1 diff --git a/GitHubConfiguration.ps1 b/GitHubConfiguration.ps1 index 7dc8656..645a84e 100644 --- a/GitHubConfiguration.ps1 +++ b/GitHubConfiguration.ps1 @@ -309,8 +309,7 @@ function Save-GitHubConfiguration ) $null = New-Item -Path $Path -Force - $Configuration | - ConvertTo-Json | + ConvertTo-Json -InputObject $Configuration | Set-Content -Path $Path -Force -ErrorAction SilentlyContinue -ErrorVariable ev if (($null -ne $ev) -and ($ev.Count -gt 0)) @@ -654,7 +653,7 @@ function Backup-GitHubConfiguration } else { - @{} | ConvertTo-Json | Set-Content -Path $Path -Force:$Force + ConvertTo-Json -InputObject @{} | Set-Content -Path $Path -Force:$Force } } diff --git a/GitHubCore.ps1 b/GitHubCore.ps1 index a6f8948..21f6dad 100644 --- a/GitHubCore.ps1 +++ b/GitHubCore.ps1 @@ -261,7 +261,7 @@ function Invoke-GHRestMethod Write-Log -Message "Unable to retrieve the raw HTTP Web Response:" -Exception $_ -Level Warning } - throw ($ex | ConvertTo-Json -Depth 20) + throw (ConvertTo-Json -InputObject $ex -Depth 20) } } @@ -840,8 +840,12 @@ filter ConvertTo-SmarterObject .PARAMETER InputObject The object to update #> + [CmdletBinding()] param( - [Parameter(Mandatory)] + [Parameter( + Mandatory, + ValueFromPipeline, + ValueFromPipelineByPropertyName)] [AllowNull()] [object] $InputObject ) @@ -851,31 +855,44 @@ filter ConvertTo-SmarterObject return $null } - if ($InputObject -is [array]) + if ($InputObject -is [System.Collections.IList]) { - foreach ($object in $InputObject) - { - Write-Output -InputObject (ConvertTo-SmarterObject -InputObject $object) - } + $InputObject | + ConvertTo-SmarterObject | + Write-Output } elseif ($InputObject -is [PSCustomObject]) { - $properties = $InputObject.PSObject.Properties | Where-Object { $null -ne $_.Value } + $clone = DeepCopy-Object -InputObject $InputObject + $properties = $clone.PSObject.Properties | Where-Object { $null -ne $_.Value } foreach ($property in $properties) { # Convert known date properties from dates to real DateTime objects - if ($property.Name -in $script:datePropertyNames) + if (($property.Name -in $script:datePropertyNames) -and + ($property.Value -is [String]) -and + (-not [String]::IsNullOrWhiteSpace($property.Value))) { - $property.Value = Get-Date -Date $property.Value + try + { + $property.Value = Get-Date -Date $property.Value + } + catch + { + Write-Log -Message "Unable to convert $($property.Name) value of $($property.Value) to a [DateTime] object. Leaving as-is." -Level Verbose + } } - if (($property.Value -is [array]) -or ($property.Value -is [PSCustomObject])) + if ($property.Value -is [System.Collections.IList]) + { + $property.Value = @(ConvertTo-SmarterObject -InputObject $property.Value) + } + elseif ($property.Value -is [PSCustomObject]) { $property.Value = ConvertTo-SmarterObject -InputObject $property.Value } } - Write-Output -InputObject $InputObject + Write-Output -InputObject $clone } else { diff --git a/GitHubIssues.ps1 b/GitHubIssues.ps1 index 0db4a83..62767cb 100644 --- a/GitHubIssues.ps1 +++ b/GitHubIssues.ps1 @@ -514,7 +514,7 @@ function New-GitHubIssue $params = @{ 'UriFragment' = "/repos/$OwnerName/$RepositoryName/issues" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Post' 'Description' = "Creating new Issue ""$Title"" on $RepositoryName" 'AcceptHeader' = 'application/vnd.github.symmetra-preview+json' @@ -654,7 +654,7 @@ function Update-GitHubIssue $params = @{ 'UriFragment' = "/repos/$OwnerName/$RepositoryName/issues/$Issue" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Patch' 'Description' = "Updating Issue #$Issue on $RepositoryName" 'AcceptHeader' = 'application/vnd.github.symmetra-preview+json' @@ -760,7 +760,7 @@ function Lock-GitHubIssue $params = @{ 'UriFragment' = "/repos/$OwnerName/$RepositoryName/issues/$Issue/lock" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Put' 'Description' = "Locking Issue #$Issue on $RepositoryName" 'AcceptHeader' = 'application/vnd.github.sailor-v-preview+json' diff --git a/GitHubLabels.ps1 b/GitHubLabels.ps1 index 623443f..b66be94 100644 --- a/GitHubLabels.ps1 +++ b/GitHubLabels.ps1 @@ -210,7 +210,7 @@ function New-GitHubLabel $params = @{ 'UriFragment' = "repos/$OwnerName/$RepositoryName/labels" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Post' 'Description' = "Creating label $Name in $RepositoryName" 'AcceptHeader' = 'application/vnd.github.symmetra-preview+json' @@ -425,7 +425,7 @@ function Update-GitHubLabel $params = @{ 'UriFragment' = "repos/$OwnerName/$RepositoryName/labels/$Name" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Patch' 'Description' = "Updating label $Name" 'AcceptHeader' = 'application/vnd.github.symmetra-preview+json' diff --git a/GitHubMiscellaneous.ps1 b/GitHubMiscellaneous.ps1 index ffde560..b67671f 100644 --- a/GitHubMiscellaneous.ps1 +++ b/GitHubMiscellaneous.ps1 @@ -150,7 +150,7 @@ function ConvertFrom-Markdown $params = @{ 'UriFragment' = 'markdown' - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Post' 'Description' = "Converting Markdown to HTML" 'AccessToken' = $AccessToken diff --git a/GitHubRepositories.ps1 b/GitHubRepositories.ps1 index 12686c0..b4ea3a2 100644 --- a/GitHubRepositories.ps1 +++ b/GitHubRepositories.ps1 @@ -166,7 +166,7 @@ function New-GitHubRepository $params = @{ 'UriFragment' = $uriFragment - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Post' 'Description' = "Creating $RepositoryName" 'AccessToken' = $AccessToken @@ -609,7 +609,7 @@ function Update-GitHubRepository $params = @{ 'UriFragment' = "repos/$OwnerName/$ReposistoryName" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Patch' 'Description' = "Updating $RepositoryName" 'AccessToken' = $AccessToken @@ -817,7 +817,7 @@ function Set-GitHubRepositoryTopic $params = @{ 'UriFragment' = "repos/$OwnerName/$RepositoryName/topics" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Put' 'Description' = $description 'AcceptHeader' = 'application/vnd.github.mercy-preview+json' @@ -1299,7 +1299,7 @@ function Move-GitHubRepositoryOwnership $params = @{ 'UriFragment' = "repos/$OwnerName/$RepositoryName/transfer" - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Method' = 'Post' 'Description' = "Transferring ownership of $RepositoryName to $NewOwnerName" 'AccessToken' = $AccessToken diff --git a/GitHubUsers.ps1 b/GitHubUsers.ps1 index 557f0ac..9a1c978 100644 --- a/GitHubUsers.ps1 +++ b/GitHubUsers.ps1 @@ -259,7 +259,7 @@ function Update-GitHubCurrentUser $params = @{ 'UriFragment' = 'user' 'Method' = 'Patch' - 'Body' = ($hashBody | ConvertTo-Json) + 'Body' = (ConvertTo-Json -InputObject $hashBody) 'Description' = "Updating current authenticated user" 'AccessToken' = $AccessToken 'TelemetryEventName' = $MyInvocation.MyCommand.Name diff --git a/Helpers.ps1 b/Helpers.ps1 index d801ad9..5c2247c 100644 --- a/Helpers.ps1 +++ b/Helpers.ps1 @@ -467,7 +467,7 @@ function Write-InvocationLog } else { - $params += "-$($param.Key) $($param.Value | ConvertTo-Json -Depth $jsonConversionDepth -Compress)" + $params += "-$($param.Key) $(ConvertTo-Json -InputObject $param.Value -Depth $jsonConversionDepth -Compress)" } } } diff --git a/PowerShellForGitHub.psd1 b/PowerShellForGitHub.psd1 index 098e5aa..2b2fa39 100644 --- a/PowerShellForGitHub.psd1 +++ b/PowerShellForGitHub.psd1 @@ -11,7 +11,7 @@ Description = 'PowerShell wrapper for GitHub API' # Script module or binary module file associated with this manifest. - # RootModule = 'GitHubCore.psm1' + RootModule = 'PowerShellForGitHub.psm1' # Modules to import as nested modules of the module specified in RootModule/ModuleToProcess NestedModules = @( diff --git a/PowerShellForGitHub.psm1 b/PowerShellForGitHub.psm1 new file mode 100644 index 0000000..9380798 --- /dev/null +++ b/PowerShellForGitHub.psm1 @@ -0,0 +1,4 @@ +# This file only exists to enable making this a "Script" module instead of a "Manifest" module +# so that Pester tests are able to use Pester's Mock and InModuleScope features for internal +# methods. For more information, refer to: +# https://github.com/pester/Pester/wiki/Testing-different-module-types \ No newline at end of file diff --git a/Tests/GitHubCore.Tests.ps1 b/Tests/GitHubCore.Tests.ps1 new file mode 100644 index 0000000..d806e43 --- /dev/null +++ b/Tests/GitHubCore.Tests.ps1 @@ -0,0 +1,267 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +<# +.Synopsis + Tests for GitHubCore.ps1 module +#> + +[String] $root = Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path) +. (Join-Path -Path $root -ChildPath 'Tests\Config\Settings.ps1') +Import-Module -Name $root -Force + +function Initialize-AppVeyor +{ +<# + .SYNOPSIS + Configures the tests to run with the authentication information stored in AppVeyor + (if that information exists in the environment). + + .DESCRIPTION + Configures the tests to run with the authentication information stored in AppVeyor + (if that information exists in the environment). + + The Git repo for this module can be found here: http://aka.ms/PowerShellForGitHub + + .NOTES + Internal-only helper method. + + The only reason this exists is so that we can leverage CodeAnalysis.SuppressMessageAttribute, + which can only be applied to functions. + + We call this immediately after the declaration so that AppVeyor initialization can heppen + (if applicable). + +#> + [CmdletBinding()] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingConvertToSecureStringWithPlainText", "", Justification="Needed to configure with the stored, encrypted string value in AppVeyor.")] + param() + + if ($env:AppVeyor) + { + $secureString = $env:avAccessToken | ConvertTo-SecureString -AsPlainText -Force + $cred = New-Object System.Management.Automation.PSCredential "", $secureString + Set-GitHubAuthentication -Credential $cred + + $script:ownerName = $env:avOwnerName + $script:organizationName = $env:avOrganizationName + + $message = @( + 'This run is executed in the AppVeyor environment.', + 'The GitHub Api Token won''t be decrypted in PR runs causing some tests to fail.', + '403 errors possible due to GitHub hourly limit for unauthenticated queries.', + 'Use Set-GitHubAuthentication manually. modify the values in Tests\Config\Settings.ps1,', + 'and run tests on your machine first.') + Write-Warning -Message ($message -join [Environment]::NewLine) + } +} + +Initialize-AppVeyor + +$script:accessTokenConfigured = Test-GitHubAuthenticationConfigured +if (-not $script:accessTokenConfigured) +{ + $message = @( + 'GitHub API Token not defined, some of the tests will be skipped.', + '403 errors possible due to GitHub hourly limit for unauthenticated queries.') + Write-Warning -Message ($message -join [Environment]::NewLine) +} + +try +{ + # Backup the user's configuration before we begin, and ensure we're at a pure state before running + # the tests. We'll restore it at the end. + $configFile = New-TemporaryFile + Backup-GitHubConfiguration -Path $configFile + Reset-GitHubConfiguration + Set-GitHubConfiguration -DisableTelemetry # We don't want UT's to impact telemetry + + Describe 'Testing ConvertTo-SmarterObject behavior' { + InModuleScope PowerShellForGitHub { + $jsonConversionDepth = 20 + + Context 'When a property is a simple type' { + $original = [PSCustomObject]@{ + 'prop1' = 'value1' + 'prop2' = 3 + 'prop3' = $null + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should return the same values' { + $originalJson = (ConvertTo-Json -InputObject $original -Depth $jsonConversionDepth) + $convertedJson = (ConvertTo-Json -InputObject $converted -Depth $jsonConversionDepth) + $originalJson -eq $convertedJson | Should be $true + } + } + + Context 'When a property is a PSCustomObject' { + $original = [PSCustomObject]@{ + 'prop1' = [PSCustomObject]@{ + 'prop1' = 'value1' + 'prop2' = 3 + 'prop3' = $null + } + 'prop2' = 3 + 'prop3' = $null + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should return the correct values' { + $originalJson = (ConvertTo-Json -InputObject $original -Depth $jsonConversionDepth) + $convertedJson = (ConvertTo-Json -InputObject $converted -Depth $jsonConversionDepth) + $originalJson -eq $convertedJson | Should be $true + } + } + + Context 'When a known date property has a date string' { + $date = Get-Date + $dateString = $date.ToUniversalTime().ToString('o') + $original = [PSCustomObject]@{ + 'prop1' = $dateString + 'closed_at' = $dateString + 'committed_at' = $dateString + 'completed_at' = $dateString + 'created_at' = $dateString + 'date' = $dateString + 'due_on' = $dateString + 'last_edited_at' = $dateString + 'last_read_at' = $dateString + 'merged_at' = $dateString + 'published_at' = $dateString + 'pushed_at' = $dateString + 'starred_at' = $dateString + 'started_at' = $dateString + 'submitted_at' = $dateString + 'timestamp' = $dateString + 'updated_at' = $dateString + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should convert the value to a [DateTime]' { + $converted.closed_at -is [DateTime] | Should be $true + $converted.committed_at -is [DateTime] | Should be $true + $converted.completed_at -is [DateTime] | Should be $true + $converted.created_at -is [DateTime] | Should be $true + $converted.date -is [DateTime] | Should be $true + $converted.due_on -is [DateTime] | Should be $true + $converted.last_edited_at -is [DateTime] | Should be $true + $converted.last_read_at -is [DateTime] | Should be $true + $converted.merged_at -is [DateTime] | Should be $true + $converted.published_at -is [DateTime] | Should be $true + $converted.pushed_at -is [DateTime] | Should be $true + $converted.starred_at -is [DateTime] | Should be $true + $converted.started_at -is [DateTime] | Should be $true + $converted.submitted_at -is [DateTime] | Should be $true + $converted.timestamp -is [DateTime] | Should be $true + $converted.updated_at -is [DateTime] | Should be $true + } + + It 'Should NOT convert the value to a [DateTime] if it''s not a known property' { + $converted.prop1 -is [DateTime] | Should be $false + } + } + + Context 'When a known date property has a null, empty or invalid date string' { + $original = [PSCustomObject]@{ + 'closed_at' = $null + 'committed_at' = '123' + 'completed_at' = '' + 'created_at' = 123 + 'date' = $null + 'due_on' = '123' + 'last_edited_at' = '' + 'last_read_at' = 123 + 'merged_at' = $null + 'published_at' = '123' + 'pushed_at' = '' + 'starred_at' = 123 + 'started_at' = $null + 'submitted_at' = '123' + 'timestamp' = '' + 'updated_at' = 123 + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should keep the existing value' { + $original.closed_at -eq $converted.closed_at | Should be $true + $original.committed_at -eq $converted.committed_at | Should be $true + $original.completed_at -eq $converted.completed_at | Should be $true + $original.created_at -eq $converted.created_at | Should be $true + $original.date -eq $converted.date | Should be $true + $original.due_on -eq $converted.due_on | Should be $true + $original.last_edited_at -eq $converted.last_edited_at | Should be $true + $original.last_read_at -eq $converted.last_read_at | Should be $true + $original.merged_at -eq $converted.merged_at | Should be $true + $original.published_at -eq $converted.published_at | Should be $true + $original.pushed_at -eq $converted.pushed_at | Should be $true + $original.starred_at -eq $converted.starred_at | Should be $true + $original.started_at -eq $converted.started_at | Should be $true + $original.submitted_at -eq $converted.submitted_at | Should be $true + $original.timestamp -eq $converted.timestamp | Should be $true + $original.updated_at -eq $converted.updated_at | Should be $true + } + } + + Context 'When an object has an empty array' { + $original = [PSCustomObject]@{ + 'prop1' = 'value1' + 'prop2' = 3 + 'prop3' = @() + 'prop4' = $null + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should still be an empty array after conversion' { + $originalJson = (ConvertTo-Json -InputObject $original -Depth $jsonConversionDepth) + $convertedJson = (ConvertTo-Json -InputObject $converted -Depth $jsonConversionDepth) + $originalJson -eq $convertedJson | Should be $true + } + } + + Context 'When an object has a single item array' { + $original = [PSCustomObject]@{ + 'prop1' = 'value1' + 'prop2' = 3 + 'prop3' = @(1) + 'prop4' = $null + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should still be a single item array after conversion' { + $originalJson = (ConvertTo-Json -InputObject $original -Depth $jsonConversionDepth) + $convertedJson = (ConvertTo-Json -InputObject $converted -Depth $jsonConversionDepth) + $originalJson -eq $convertedJson | Should be $true + } + } + + Context 'When an object has a multi-item array' { + $original = [PSCustomObject]@{ + 'prop1' = 'value1' + 'prop2' = 3 + 'prop3' = @(1, 2) + 'prop4' = $null + } + + $converted = ConvertTo-SmarterObject -InputObject $original + + It 'Should still be a multi item array after conversion' { + $originalJson = (ConvertTo-Json -InputObject $original -Depth $jsonConversionDepth) + $convertedJson = (ConvertTo-Json -InputObject $converted -Depth $jsonConversionDepth) + $originalJson -eq $convertedJson | Should be $true + } + } + } + } +} +finally +{ + # Restore the user's configuration to its pre-test state + Restore-GitHubConfiguration -Path $configFile +}