Rule 'PSDSCUseIdenticalMandatoryParametersForDSC' throws incorrect error

[ykuijs]

Steps to reproduce

Since the recent release, several tests in the SharePointDsc module fail with the following error:
The following PSScriptAnalyzer rule 'PSDSCUseIdenticalMandatoryParametersForDSC' errors need to be fixed:
MSFT_SPSearchContentSource.psm1 (Line 1): The 'Required' parameter 'ScheduleType' is not present in 'Get-TargetResource' DSC resource function(s).
MSFT_SPSearchContentSource.psm1 (Line 219): The 'Required' parameter 'ScheduleType' is not present in 'Set-TargetResource' DSC resource function(s).
MSFT_SPSearchContentSource.psm1 (Line 683): The 'Required' parameter 'ScheduleType' is not present in 'Test-TargetResource' DSC resource function(s).

The specified parameter however are parameters in subclasses, which should not be present in the *-TargetResource methods.

[ClassVersion("1.0.0")]
Class MSFT_SPSearchCrawlSchedule
{
    [Required, Description("How frequently should this crawl be run"), ValueMap{"None","Daily","Weekly","Monthly"}, Values{"None","Daily","Weekly","Monthly"}] String ScheduleType;
    [Write, Description("Monthly crawls only: Which day of the month should the crawl run on")] Uint32 CrawlScheduleDaysOfMonth;
    [Write, Description("Weekly crawls only: What days should the crawl be run on"), ValueMap{"Everyday", "Weekdays", "Weekends", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"}, Values{"Everyday", "Weekdays", "Weekends", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"}] String CrawlScheduleDaysOfWeek[];
    [Write, Description("Monthly crawls only: Which months should this crawl be run during"), ValueMap{"AllMonths", "January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"}, Values{"AllMonths", "January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"}] String CrawlScheduleMonthsOfYear[];
    [Write, Description("The hour of the day that the schedule should begin")] Uint32 StartHour;
    [Write, Description("The minute of the specified hour that the schedule should begin")] Uint32 StartMinute;
    [Write, Description("Specifies the number of times to repeat the crawl within a day")] Uint32 CrawlScheduleRepeatDuration;
    [Write, Description("Specifies the number of minutes between crawl repeats on a day")] Uint32 CrawlScheduleRepeatInterval;
    [Write, Description("For daily crawls, this is the number of days between crawls. For weekly this is the number of weeks between crawls")] Uint32 CrawlScheduleRunEveryInterval;
};

[ClassVersion("1.0.0.0"), FriendlyName("SPSearchContentSource")]
class MSFT_SPSearchContentSource : OMI_BaseResource
{
    [Key, Description("The name of the content source")] String Name;
    [Key, Description("The name of the search service app that this content source exists within")] String ServiceAppName;
    [Required, Description("The type of content source - currently only SharePoint, Website, File Shares and Business are supported"), ValueMap{"SharePoint","Website","FileShare","Business"}, Values{"SharePoint","Website","FileShare","Business"}] String ContentSourceType;
    [Write, Description("A list of the addresses this content source includes")] String Addresses[];
    [Write, Description("Should the crawler index everything, just the first site or page, or a custom depth (applies to websites only)"), ValueMap{"CrawlEverything","CrawlFirstOnly","Custom"}, Values{"CrawlEverything","CrawlFirstOnly","Custom"}] String CrawlSetting;
    [Write, Description("Should this content source use continuous crawl (SharePoint sites only)")] Boolean ContinuousCrawl;
    [Write, Description("What is the incremental schedule for this content source"), EmbeddedInstance("MSFT_SPSearchCrawlSchedule")] String IncrementalSchedule;
    [Write, Description("What is the full schedule for this content source"), EmbeddedInstance("MSFT_SPSearchCrawlSchedule")] String FullSchedule;
    [Write, Description("What is the priority on this content source"), ValueMap{"Normal","High"}, Values{"Normal","High"}] String Priority;
    [Write, Description("How many pages deep should the crawler go (-1 = unlimited, website sources only)")] Uint32 LimitPageDepth;
    [Write, Description("How many server hops should the crawler make (-1 = unlimtied, website sources only)")] Uint32 LimitServerHops;
    [Write, Description("Line of Business System and System Instance names")] String LOBSystemSet[];
    [Write, Description("Present if the source should exist, absent if it should not"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] string Ensure;
    [Write, Description("Specify true if DSC is allowed to delete and recreate a content source to apply the correct settings, otherwise false will just report errors if a change can not be applied.")] Boolean Force;
    [Write, Description("POWERSHELL 4 ONLY: The account to run this resource as, use PsDscRunAsCredential if using PowerShell 5"), EmbeddedInstance("MSFT_Credential")] String InstallAccount;
};

Expected behavior

Not complaint about the missing parameters

Actual behavior

Gives false positives:

MSFT_SPSearchContentSource.psm1 (Line 1): The 'Required' parameter 'ScheduleType' is not present in 'Get-TargetResource' DSC resource function(s).
MSFT_SPSearchContentSource.psm1 (Line 219): The 'Required' parameter 'ScheduleType' is not present in 'Set-TargetResource' DSC resource function(s).
MSFT_SPSearchContentSource.psm1 (Line 683): The 'Required' parameter 'ScheduleType' is not present in 'Test-TargetResource' DSC resource function(s).

Environment data

Tests are running in AppVeyor with PSv5.1 and v1.18 of PSScriptAnalyzer

[bergmeister]

@ykuijs Thanks for reporting it. I can repro and approve that behaviour changed between 1.17.1 and 1.18.0 and I could already boil it down to a one line change in #1046
I need to take some time though to understand your use case to say if the behaviour is by design or not. I'd appreciate if you could try to explain it in a bit more detail please or in a more minimal example (that would be useful to write a test case for it, which we need to do for every fix anyway)

[ykuijs]

For several resources in SharePointDsc are we using subclasses, for example https://github.com/PowerShell/SharePointDsc/tree/dev/Modules/SharePointDsc/DSCResources/MSFT_SPExcelServiceApp

As you can see in the schema of SPExcelServiceApp, the MSFT_SPExcelFileLocation class is defined in the schema with a key parameter called Address. However this class is used as EmbeddedInstance for the parameter TrustedFileLocations. The TrustedFileLocations parameter IS declared in all *-TargetResource methods.

In PSSA v1.17 this setup was fine, but v1.18 PSSA now throws an error for all parameters in the subclass that are specified as "Key" or "Required" that they are not present in the *-TargetResource methods, which they are not supposed to.

Since we "borrowed" this setup from xWebsite in xWebAdministration, I guess tests in new PRs for this resource will fail as well.

I hope this explains the issue a little bit more. Let me know if you need more info!

[ykuijs]

FYI: As a workaround (so our PRs aren't blocked) I have disabled this specific test for the affected resources. If you fork SharePointDsc and run Invoke-ScriptAnalyzer on one of the resources to test/reproduce, you first have to remove the following line from all methods:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSDSCUseIdenticalMandatoryParametersForDSC", "", Justification  =  "Temporary workaround for issue introduced in PSSA v1.18")]

[bergmeister]

Hmm, actually, by repro-ing locally (by cd-ing to the MSFT_SPSearchContentSource folder and calling Invoke-ScriptAnalyzer -Path .), it shows that in 1.17.1, PSSA threw a NullReferenceException, which got fixed by #1046, therefore the code can actually proceed further in 1.18.0, so I think the code change itself was not a regression in the sense that it was not causing this bug but it rather allowed the bug to be exposed. It seems that NullReference error (which might've been a non-terminating error) was swallowed by your CI tests. I am not very familiar with DSC and the person that wrote the rule, is not a maintainer any more. I'd appreciate it if you take a stab at looking at the logic of the rule here in the meantime if you want a quick fix, maybe you can see the fault in the logic? Most of the logic in this repo was written in a minimum, viable way so it might be the case that the logic is too simple and needs to be enhanced... Building this repo is very easy, you just run build.ps1; Import-Module .\out\PSScriptAnalyzer\PSScriptAnalyzer.psd1 and then you can call Invoke-ScriptAnalyzer and you can just attach Visual Studio to the PowerShell process to debug the code in that rule.

[ykuijs]

Just tried to reproduce and you are absolutely correct. I also get the NullReferenceException with v1.17.1. I am not very good with C# (I am an IT Pro guy, with a small Dev background :-)), but will try take a look at it and hopefully can figure out what needs to get changed.

[ykuijs]

I have found where the issue is coming from:
On line 259 of the UseIdenticalMandatoryParametersDSC.cs, the first class in the file is taken and the required parameters extracted:

PSScriptAnalyzer/Rules/UseIdenticalMandatoryParametersDSC.cs

Lines 259 to 274 in 23a18f6

	var cimClass = cimClasses?.FirstOrDefault();
	var cimSuperClassProperties = new HashSet<string>(
	cimClass?.CimSuperClass?.CimClassProperties.Select(cimPropertyDeclaration => cimPropertyDeclaration.Name) ??
	Enumerable.Empty<string>());
	return cimClass?
	.CimClassProperties?
	.Where(p => (p.Flags.HasFlag(CimFlags.Key) ||
	p.Flags.HasFlag(CimFlags.Required)) &&
	!cimSuperClassProperties.Contains(p.Name))
	.ToDictionary(
	p => p.Name,
	p => p.Flags.HasFlag(CimFlags.Key) ?
	CimFlags.Key.ToString() :
	CimFlags.Required.ToString()) ??
	emptyDictionary;

In our case, but also in xWebsite, the first class in the file is the subclass. When switching the order (placing the main class first), the rule isn't triggered.

Now for the solution:
Even though changing the order of the classes works, I think this is a workaround.

What I think should happen, is that the code should select the class which has "OMI_BaseResource" as CimSuperClassName instead of simply the first one:
https://github.com/PowerShell/SharePointDsc/blob/b1a38f6eb08757d99450aa6209989ed94a998d25/Modules/SharePointDsc/DSCResources/MSFT_SPExcelServiceApp/MSFT_SPExcelServiceApp.schema.mof

Unfortunately my C# skills aren't good enough to implement this myself. @bergmeister, can you assist with implementing this?

[bergmeister]

Awesome work. Yes, I'm happy to start implementing a fix to that, I will tag you in the PR when I'll open it (I hope in 1-2 days). This is a great example of team work :-)

[bergmeister]

@ykuijs PSSA 1.18.1 has finally been released :-)
https://devblogs.microsoft.com/powershell/release-of-powershell-script-analyzer-1-18-1/

[ykuijs]

@bergmeister Thanks for letting me know!