Instead of using the first cimClass and then having no superClass, use the first cimClass that has a non-null superClass

[bergmeister]

PR Summary

Fixes #1192

Instead of using the first cimClass and then having no superClass, use the first cimClass that has a non-null superClass.
Implementation is ready but we need to write a test for it I suppose.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[bergmeister]

@ykuijs This would be a general improvement and would fix your issue because the first cimClass has no Superclass, therefore, it will pick the 2nd cimClass, which has a Superclass. What do you think? I still have to look into whether we can do something better rather than selecting the first superClass and rather try to get the expected name so that we can match against it or do you think we don't need it. It would be great if you could maybe come up with a more minimal example that we could use as a regression test, which is a minimum requirement.

[ykuijs]

I agree that this should work. xWebsite is using three sub-CimClasses, all without SuperClass. So I guess this should skip all three and take the xWebSite class.

https://github.com/PowerShell/xWebAdministration/blob/dev/DSCResources/MSFT_xWebsite/MSFT_xWebsite.schema.mof

[ykuijs]

To get back to your question about a test scenario:
If you update the schema of the DSCResource example, with the following code, the test should not complain about a required variable in one of the *-TargetResource methods:

#pragma namespace("\\\\.\\root\\microsoft\\windows\\DesiredStateConfiguration")

[ClassVersion("1.0.0.0")]
Class MSFT_SubClass
{
    [Key, Description("Key of the subclass")] String Name;
    [Required, Description("Required parameter of the subclass")] String Description;
    [Write, Description("Additional non-required parameter")] Boolean Enabled;
};

[ClassVersion("1.0.0"), FriendlyName("WaitForAny")]
class MSFT_WaitForAnyNoIdenticalMandatoryParameter : OMI_BaseResource
{
    [key, Description("Name of Resource on remote machine")]
    string ResourceName;

    [key, Description("dummy variable")]
    string Dummy;

    [required, Description("List of remote machines")]
    string NodeName[];

    [required, EmbeddedInstance("MSFT_Credential"), Description("Credential to access all remote machines")]
    String Credential;

    [write, Description("dummy subclass variable"), EmbeddedInstance("MSFT_SubClass")]
    string Subclass;

    [write, Description("Time between various retries. Lower bound is 1.")]
    Uint64 RetryIntervalSec;

    [write, Description("Maximum number of retries to check the state of resource.")]
    Uint32 RetryCount;

    [write, Description("Number of machines to connect simultaneously. Default is new-cimsession default")]
    Uint32 ThrottleLimit;

    [read, Description("List of remote machines in desired state.")]
    String NodesInDesiredState;

};

[ykuijs]

Any update on the progress of this PR?

[bergmeister]

I think the PR is OK, it just needs a test, I'll add reviewers to speedup the process for the moment

[ykuijs]

If you add this test to Tests\Rules\UseIdenticalMandatoryParametersForDSC.tests.ps1, this should test the change:

    Context "When a CIM class has no parent, but does contain a subclass which should not be processed" {
        # regression test for #1192 - just check no uncaught exception
        It "Should find no violations, and throw no exceptions" -skip:($IsLinux -or $IsMacOS) {

            # Arrange test content in testdrive
            $dscResources = Join-Path -Path "TestDrive:" -ChildPath "DSCResources"
            $noparentClassDir = Join-Path -Path $dscResources "ClassWithNoParent"

            # need a fake module
            $fakeModulePath = Join-Path -Path "TestDrive:" -ChildPath "test.psd1"
            Set-Content -Path $fakeModulePath -Value @"
@{
    ModuleVersion = '1.0'
    GUID = 'f5e6cc2a-5500-4592-bbe2-ef033754b56f'
    Author = 'test'

    FunctionsToExport = @()
    CmdletsToExport = @()
    VariablesToExport = '*'
    AliasesToExport = @()

    # Private data to pass to the module specified in RootModule/ModuleToProcess. This may also contain a PSData hashtable with additional module metadata used by PowerShell.
    PrivateData = @{
        PSData = @{
        } # End of PSData hashtable
    } # End of PrivateData hashtable
}
"@
            # and under it a directory called dscresources\something
            New-Item -ItemType Directory -Path $noParentClassDir
            $noparentClassFilepath = Join-Path -Path $noParentClassDir -ChildPath 'ClassWithNoParent.psm1'
            $noparentClassMofFilepath = Join-Path -Path $noParentClassDir -ChildPath 'ClassWithNoParent.schema.mof'

            # containing a .psm1 file and a .schema.mof file with same base name
            Set-Content -Path $noParentClassFilepath -Value "#requires -Version 4.0 -Modules CimCmdlets" # the file content doesn't much matter

            Set-Content -Path $noParentClassMofFilePath -Value @"
[ClassVersion("1.0.0.0")]
Class MSFT_SubClass
{
    [Key, Description("Key of the subclass")] String Name;
    [Required, Description("Required parameter of the subclass")] String Description;
    [Write, Description("Additional non-required parameter")] Boolean Enabled;
};

[ClassVersion("1.0.0"), FriendlyName("WaitForAny")]
class MSFT_WaitForAnyNoIdenticalMandatoryParameter : OMI_BaseResource
{
    [key, Description("Name of Resource on remote machine")]
    string ResourceName;

    [key, Description("dummy variable")]
    string Dummy;

    [required, Description("List of remote machines")]
    string NodeName[];

    [required, EmbeddedInstance("MSFT_Credential"), Description("Credential to access all remote machines")]
    String Credential;

    [write, Description("dummy subclass variable"), EmbeddedInstance("MSFT_SubClass")]
    string Subclass;
};
"@

            # Act - run scriptanalyzer
            $violations = Invoke-ScriptAnalyzer -Path $noParentClassFilepath -IncludeRule $ruleName -ErrorAction Stop
            $violations.Count | Should -Be 0
        }
    }

[bergmeister]

@ykuijs Thanks for all your efforts.. :-)
I tried using your test but the test does not fail for me when run against 1.18.0 somehow, I tried looking into it but couldn't find out why. I then looked at your original MSFT_SPSearchContentSource folder (I still have the local clone before your changes when you raised the issue initially) and when I run Invoke-ScriptAnalyzer . in that folder, I can repro, but if I copy that folder to a different directory outside of your DSC git repo, I cannot repro any more, which suggests that something outside its folder higher up is influencing it and then makes the bug not appear any more. I am unfortunately lacking the deeper knowledge of DSC, it would be great if you could look into this please. If this turns out to be too hard, we can decide to just take the PR without a test but we should make some effort to have a test that would fail in case of a regression.

[ykuijs]

DSC requires a specific folder structure, so if you take the resource out of that folder structure I believe PSSA no longer sees it as a DSC module and therefore does not run the test. Let me dive into the test a little deeper, I have some cycles available this Thursday.

[ykuijs]

A little later than planned, but I just managed to test the new test with v1.18.0 and there it fails as expected:

PSDSCStandardDSCFunctionsInResource Error        ClassWithN 1     Missing 'Get-TargetResource' function. DSC Resource must imp
                                                 oParent.ps       lement Get, Set and Test-TargetResource functions.          
                                                 m1                                                                           
PSDSCStandardDSCFunctionsInResource Error        ClassWithN 1     Missing 'Set-TargetResource' function. DSC Resource must imp
                                                 oParent.ps       lement Get, Set and Test-TargetResource functions.          
                                                 m1                                                                           
PSDSCStandardDSCFunctionsInResource Error        ClassWithN 1     Missing 'Test-TargetResource' function. DSC Resource must im
                                                 oParent.ps       plement Get, Set and Test-TargetResource functions.         
                                                 m1                                                                           

So the test is good for testing this scenario.

[bergmeister]

@ykuijs When I paste your test into the end of Tests/Rules/UseIdenticalMandatoryParametersForDSC.tests.ps1 then the test passes on PowerShell 5.1 and 6.2 for me, maybe something is different between your and my machine. Can you please open a PR for the test please so that we see the test failing once please? Once we can be sure that the test fails, we can then merge both PRs. Thanks for your patience

[ykuijs]

No sure what you mean here. The Test passes for me as well with the fixes from this PR. However if I generate the example resource using the code in the test and run the v1.18.0 from the PSGallery, I get the above errors.

[ykuijs]

Sorry, now see I did something wrong.....continuing testing!

[bergmeister]

@ykuijs Although the test should pass WITH the fix (which it does, so no problem here), but for a test to be meaningful, it must also fail when the fix is NOT applied and PR #1231 shows that it currently does not fail (and I even tried the 1.18 version from the PSGallery locally as well)

[ykuijs]

[UPDATED the test because of a copy/paste error]

@bergmeister That makes perfect sense. I tried to reproduce the error using v1.18.0, but indeed was unable to. It took me a while, but I finally managed to figure it out :-) I copied the test code from several places, but made a small mistake while doing this. DSC requires the filename to match the classname, which it didn't. Therefore these tests were not processed correctly.

I tested the following test which fails on v1.18.0 and passes with the updated code:

    Context "When a CIM class has no parent, but does contain a subclass which should not be processed" {
        # regression test for #1192 - just check no uncaught exception
        It "Should find no violations, and throw no exceptions" -skip:($IsLinux -or $IsMacOS) {

            # Arrange test content in testdrive
            $dscResources = Join-Path -Path "TestDrive:" -ChildPath "DSCResources"
            $noparentClassDir = Join-Path -Path $dscResources "ClassWithNoParent"

            # need a fake module
            $fakeModulePath = Join-Path -Path "TestDrive:" -ChildPath "test.psd1"
            Set-Content -Path $fakeModulePath -Value @"
@{
    ModuleVersion = '1.0'
    GUID = 'f5e6cc2a-5500-4592-bbe2-ef033754b56f'
    Author = 'test'

    FunctionsToExport = @()
    CmdletsToExport = @()
    VariablesToExport = '*'
    AliasesToExport = @()

    # Private data to pass to the module specified in RootModule/ModuleToProcess. This may also contain a PSData hashtable with additional module metadata used by PowerShell.
    PrivateData = @{
        PSData = @{
        } # End of PSData hashtable
    } # End of PrivateData hashtable
}
"@
            # and under it a directory called dscresources\something
            New-Item -ItemType Directory -Path $noParentClassDir
            $noparentClassFilepath = Join-Path -Path $noParentClassDir -ChildPath 'MSFT_ClassWithNoParent.psm1'
            $noparentClassMofFilepath = Join-Path -Path $noParentClassDir -ChildPath 'MSFT_ClassWithNoParent.schema.mof'

            # containing a .psm1 file and a .schema.mof file with same base name
            Set-Content -Path $noParentClassFilepath -Value @"
#requires -Version 4.0 -Modules CimCmdlets
function Get-TargetResource
{
    param()
}

function Set-TargetResource
{
    param()
}

function Test-TargetResource
{
    param()
}

Export-ModuleMember -Function *-TargetResource
"@

            Set-Content -Path $noParentClassMofFilePath -Value @"
[ClassVersion("1.0.0.0")]
Class MSFT_SubClass
{
    [Key, Description("Key of the subclass")] String Name;
    [Required, Description("Required parameter of the subclass")] String Description;
    [Write, Description("Additional non-required parameter")] Boolean Enabled;
};

[ClassVersion("1.0.0"), FriendlyName("ClassWithNoParent")]
class MSFT_ClassWithNoParent : OMI_BaseResource
{
    [key, Description("Name of Resource on remote machine")]
    string ResourceName;

    [key, Description("dummy variable")]
    string Dummy;

    [required, Description("List of remote machines")]
    string NodeName[];

    [required, EmbeddedInstance("MSFT_Credential"), Description("Credential to access all remote machines")]
    String Credential;

    [write, Description("dummy subclass variable"), EmbeddedInstance("MSFT_SubClass")]
    string Subclass;
};
"@

            # Act - run scriptanalyzer
            $violations = Invoke-ScriptAnalyzer -Path $noParentClassFilepath -IncludeRule $ruleName -ErrorAction Stop
            $violations.Count | Should -Be 0
        }
    }

[bergmeister]

@ykuijs Thanks for coming back. I can confirm that the new regression test would now actually fail as proven in PR #1231 :-)
Thanks for your endurance and commitment to make PSSA better. I will merge now.
A patch release of PSSA should come out soon but as far as I know, @JamesWTruher (who is the project lead) wants to wait for a tweak to the recent;y released compatibility rules by @rjmholt , can you confirm or add details about time frames @rjmholt as the issue that this PR fixes impacts quite a few DSC resource repos.

[ykuijs]

@bergmeister Any clue when this will be released to the PS Gallery?

[bergmeister]

@ykuijs Please ask @JamesWTruher