gaelcolas / DscBuildHelpers

10 stars 7 forks source link

Get-DscSplattedResource throws an error when a parameter is an empty array #13

Closed ykuijs closed 11 months ago

ykuijs commented 1 year ago

I am using Get-DscSplattedResource to splat Configuration Data into a DSC resource. On of these parameters is an empty array, but unfortunately this results in an error:

PSDesiredStateConfiguration\Configuration : Exception calling "ContainsKey" with "1" argument(s): "Value cannot be null.
Parameter name: key"
At C:\Temp\DSC\SplatIssue\SplatIssue\0.0.1\DSCResources\TestResource\TestResource.schema.psm1:2 char:1
+ Configuration TestResource
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Configuration], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : ArgumentNullException,Configuration

Compilation errors occurred while processing configuration 'SplatIssue'. Please review the errors reported in error stream and modify your configuration code appropriately.
At C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules\PSDesiredStateConfiguration\PSDesiredStateConfiguration.psm1:3917 char:5
+     throw $ErrorRecord
+     ~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (SplatIssue:String) [], InvalidOperationException
    + FullyQualifiedErrorId : FailToProcessConfiguration

I have been able to reproduce the issue with a very simple composite resource and DSC config. See this file. Just start the SplatIssue.ps1 script. It will first generate a successful MOF file, using an hashtable that isn't using an empty array. Then it throws an error because it uses an empty array.

ykuijs commented 1 year ago

I troubleshooted this issue and traced it back to this line: https://github.com/gaelcolas/DscBuildHelpers/blob/e98f651778725f14e0a344c843c53a4106205642/source/Public/Get-DscSplattedResource.ps1#L88

In this line, the code generates Attributes = $($Parameters['Attributes']), where the attributes property in the Parameters variable in this case is an empty array. When you run $null -eq $Parameters['Attributes'], this returns False but when you run $null -eq $($Parameters['Attributes']) this returns True.

This issue is reproduceable with an empty array: $null -eq @() returns False and $null -eq $(@()) returns True.

I managed to solve the issue in two different ways:

  1. Removing the dollar sign, since that is not really required anyways: $null = $stringBuilder.AppendLine("$PropertyName = (`$Parameters['$PropertyName'])")
  2. Checking if the code is an empty array and if so use an empty array instead of processing the parameter at runtime. But this might mean the code also has to check for an empty hashtable and other objects (haven't tested it yet):
            if ($Properties[$PropertyName] -is [System.Array] -and $Properties[$PropertyName].Count -eq 0)
            {
                $null = $stringBuilder.AppendLine("$PropertyName = @()")
            }
            else
            {
                $null = $stringBuilder.AppendLine("$PropertyName = `$(`$Parameters['$PropertyName'])")
            }

Just not sure if this is the best way to resolve the issue. @gaelcolas, @raandree What is your opinion here?

johlju commented 1 year ago

I would remove the $() and handle any potential issue that occurs from that in other PRs.

But someone else has a better understanding why it was made like that in the first place? To work with PowerShell 4.0?

ykuijs commented 1 year ago

Checked the first commit that contains this code (back in 2018) and there the code already includes the $(): https://github.com/gaelcolas/DscBuildHelpers/blob/7a976ac40b7ae4b300c13af976409953832c2ad1/DscBuildHelpers/Public/Get-DscSplattedResource.ps1#L26-L28

@gaelcolas Any clue why this is in there and what could happen if we remove it?

raandree commented 1 year ago

I am not sure why the expression syntax $() is used but removing it does not solve the issue.

I think we are facing a bigger issue here. How can we remove all attributes of a file if this config does not work

Test = @{
            DestinationPath = "C:\Temp"
            Type = 'Directory'
            Attributes = ''
        }
ykuijs commented 11 months ago

@raandree What are you doing to validate if the fix works? It works on my end. Have you reloaded the module after changing the code?

Agree with you that this issue needs to get fixed. We indeed should be able to specify empty values (empty arrays, hashtables, strings, etc) in order to set no value at all. Am running into several issues with this at a customer.

Will create a PR for this change!

raandree commented 11 months ago

I am wondering why this hasn't came up earlier. I need to build a new lab for the DscWorkshop and for Microsoft365DscWorkshop over the holidays. I can test the fix there as well as with DscConfig.M365. Especially with M365 removing values seems important.

The PR you are going to file is removing the $() syntax, right?

ykuijs commented 11 months ago

That is correct!

ykuijs commented 11 months ago

PR has been submitted: #14