Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregate exceptions when parsing a settings hashtable #1275

Conversation

travisclagrone
Copy link
Contributor

@travisclagrone travisclagrone commented Jun 25, 2019

This pull request aggregates exceptions raised about any hashtable argument to the -Settings parameter of the Invoke-ScriptAnalyzer command in order to output information about all knowable such exceptions in the same invocation (as opposed to outputting only one per invocation). Concomitantly, this pull request refines the related exception message resource strings in order to increase the specificity and granularity thereof.

The expected benefits include improving both the usefulness as well as the usability of exceptions raised about any hashtable argument to the -Settings parameter of the Invoke-ScriptAnalyzer command. Usefulness is improved via more informative (specific and granular) exception messages. Usability is improved via aggregating all knowable such exceptions, which renders any multi-exceptional -Settings hashtable argument debuggable after a single attempted execution (e.g. as part of automated testing), as opposed to requiring multiple debugging cycles to surface each such problem despite independence thereof.

Example

Given

$settings = @{
    foo = 'bar'
    CustomRulePath = $null
    IncludeRules = @(
        $null
        42
    )
}

When

Invoke-ScriptAnalyzer -ScriptDefinition 'Write-Output "Hello, world!"' -Settings $settings

Old behavior

Only one problem is ambiguously reported.

Invoke-ScriptAnalyzer : Value  for key CustomRulePath has the wrong data type.
At line:1 char:1
+ Invoke-ScriptAnalyzer -ScriptDefinition 'Write-Output "Hello, world!" ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidData: (System.Collections.Hashtable:Hashtable) [Invoke-ScriptAnalyzer], InvalidDataException
+ FullyQualifiedErrorId : SETTINGS_ERROR,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

New behavior

All detectable problems are reported.

Invoke-ScriptAnalyzer : One or more errors occurred. (The setting 'CustomRulePath' value is null.) (The setting 'IncludeRules', index 0 element is null.) (The setting 'IncludeRules', index 1 element, value '42' is not a string type.) (foo is not a valid key in the settings hashtable. Valid keys are CustomRulePath, ExcludeRules, IncludeRules, IncludeDefaultRules, RecurseCustomRulePath, Rules and Severity.)
At line:1 char:1
+ Invoke-ScriptAnalyzer -ScriptDefinition 'Write-Output "Hello, world!" ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidData: (System.Collections.Hashtable:Hashtable) [Invoke-ScriptAnalyzer], AggregateException
+ FullyQualifiedErrorId : SETTINGS_ERROR,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

Future Work

  • Flesh out formal specifications and test cases pertaining to exceptional behavior when parsing a hashtable argument to the -Settings parameter of the Invoke-ScriptAnalyzer command.
  • Generalize the concept of this pull request into a PSScriptAnalyzer rule.

…nvoke-ScriptAnalyzer -Settings' results in an exception
…ScriptAnalyzer -Settings` results in an exception
…rounding settings file load invocation"

This reverts commit 3bfd45b.
…processing the argument of `Invoke-ScriptAnalyzer -Settings` results in an exception
This reverts commit 88ab1ec.

`using System.IO` is required in Engine\Settings.cs after all. The main
use is for pre-existing unqualified references to the type
InvalidDataException.
…ings.ConvertToRuleArgumentType(Dictionary<string, object>)
…ettings.ConvertToRuleArgumentType(Dictionary<string, objct>)
…thod Strings.ConvertToRuleArgumentType(Dictionary<string, object>)
@travisclagrone travisclagrone changed the title Merge branch 'implement-AggregateException-for-ParseSettingsHashtable_Hashtable' into aggregate-settings-file-exceptions Aggregate exceptions when parsing a settings hashtable Jun 25, 2019
@travisclagrone
Copy link
Contributor Author

travisclagrone commented Jun 29, 2019

@bergmeister Would you be opposed to me redoing your most recent commit ba31f6b, "Merge branch 'master' of PowerShell/PSScriptAnalyzer into aggregate-settings-file-exceptions", as a rebase in order to simplify the preceding commit list for this pull request?

@bergmeister
Copy link
Collaborator

My last commit only pulled the latest changes from master into it to resolve the merge conflict due to the merge of PR #1263 It's up to you if you want to rebase your branch or revert my last commit

About the PR itself: Can you please describe the benefit of it from a user's perspective by giving e.g. an example.
Apart from aggregating the exceptions, a lot of the logic was refactored as well, was there a reason for it? Since the original maintainer who wrote the tool, has left and we do not have much test coverage in some areas, making that many changes makes me a bit nervous. If you could please self-review and leave some comments in the PR on the reason for the change in some areas, it would help us to review and understand the proposed changes in this PR. If this PR really makes the errors more useful or fixes bugs in the existing code, then we definitely want to consider the PR :-)

@travisclagrone travisclagrone force-pushed the aggregate-settings-file-exceptions branch 2 times, most recently from ef013b1 to a1b7a8f Compare June 30, 2019 08:29
@travisclagrone
Copy link
Contributor Author

@bergmeister I have updated the description of this pull request with a synopsis of the changes and expected benefits, as well as an illustrative example. If a more detailed code review is required, could you please provide an example for me and/or some pointers as to which region(s) need reviewing? (Without going into commentary about my previous jobs, suffice to say this will be my first "real" code review of this sort.) Many thanks in advance!

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the efforts. It looks good to me, only 2 minor comments with one being optional. I added 2 more reviewers so that at least 1 more pair of eyes double checks it as well, I did some manual smoke testing and it looks good to me.
Out of curiosity: Do you use VS-Code or Visual Studio for development? Visual Studio can auto-generate the *.Designer.cs files but not VS-Code. As far as I was aware, the old helper script New-StronglyTypedCsFileForResx.ps1 is broken but to me it looks like you've used it and it works somehow (maybe it suddenly started to work in a newer version of the .Net Core SDK...). On a related note: In the next upcoming preview 7 of the .Net Core 3 SDK, dotnet build will be able to auto-generate the resources for us.

About your 2 future items: Yes, we'd definitely welcome more test cases. Please open an issue about the PSSA rule that you plan to write for validation.

@travisclagrone
Copy link
Contributor Author

travisclagrone commented Jun 30, 2019

Ok, thank you. I will wait for any additional review before further action on my part.

Yes, I have been using VS Code with the New-StronglyTypedCsFileForResx.ps1 script. Should I stop using and/or deprecate that script?

I will see about opening an issue this coming week for each of the two "future work" items.

@bergmeister
Copy link
Collaborator

If the script worked fine for you, then there shouldn't be a problem with it, apart from occasional merge conflicts, it was only a surprise to me that the script works as I was under the impression it didn't a year ago, maybe it is just an update in the .Net SDK but once .Net Core 3 is RTM, we can use it and the build process will do that work for us automatically and then we can probably remove the files from source control...

Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing blocking here

Engine/Settings.cs Outdated Show resolved Hide resolved
return (bool) value;
}

private void ParseSettingsHashtable(Hashtable settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may have more global utility. Could this be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be made public without also making it static? If so, then I will go ahead and make the change. If not, then if this should be changed to be both public and static, then I would ask to postpone this change to a subsequent pull request in which additional related refactorings can be made. This is because--as far as I understand it--rendering a Settings object constructible via a static Parse...(Hashtable) method (i.e. a factory method) should be coupled with deprecating its constructibility from a yet-to-be-parsed Hashtable via a public constructor, and thus also refactoring any extant client logic (e.g. in the "Engine/Settings/InvokeScriptAnalyzerCommands.cs" file).

string ruleName = rule.Key as string;

if (!uniqueRuleKeys.Add(ruleName))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the first element to cause an error will move on to the next element, so if there are multiple issues, they won't be caught until the user fixes the previous break. Is there a way to keep going on in some of these cases and find all the issues in the setting?

Copy link
Contributor Author

@travisclagrone travisclagrone Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesWTruher Which case(s) in particular are you referring to? To the best of my knowledge, I have sequenced the checks here such that there is no case where a preceding failed check (that currently results in a continue) would either semantically or logically allow a subsequent check (on the same element) to be performed. The only exception I can see is perhaps the uniqueness constraint, which I will update momentarily to not continue on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c7ff89b updates the uniqueness check to not continue to the next element on failure, but rather proceed to check the current element's value as well.

Engine/Settings.cs Outdated Show resolved Hide resolved
Engine/Settings.cs Outdated Show resolved Hide resolved
@bergmeister
Copy link
Collaborator

@JamesWTruher Are you happy now with the changes that were made?

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's some interesting stuff in here, but to carry out the error collect/recover parse pattern I think the right way to do it is actually:

  • Create a new class (non singleton) specifically for parsing a settings file
  • Give it private recursive descent style methods for parsing the file and a private field for a list of exceptions
  • Give it public entry points for each variant of settings you expect, like:
    • public PssaSettings ReadSettingsFromFile(string settingsPath, out List<Exception> errors)
    • public PssaSettings ReadSettingsFromHashtable(Hashtable settingsValue, out List<Exception> errors)
  • Use pattern matching and recursive descent to read the settings and add to the errors on the fly

One example of a recursive descent reader like this is here and an example of error-aggregating validation is here.

@@ -8,6 +8,7 @@
using System.Collections.ObjectModel;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is the only change in this file, I'm assuming this isn't used and can be taken out?

@@ -296,167 +252,309 @@ private bool IsStringOrStringArray(object val)
return val == null ? false : valArr.All(x => x is string);
}

private List<string> GetData(object val, string key)
private List<string> ParseSettingValueStringOrStrings(object value, string settingName, IList<Exception> exceptions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a method with StringOrStrings in it is a strong indicator that this method should be broken into an overload for string and overload for IReadOnlyList<string> and an entry point overload for object that uses a pattern match switch.

Also Parse doesn't really indicate well what's going on, since we're not trying to read this from a string. I think this a Convert or similar.

So I'd imagine this being written like this:

private List<string> GetSettingFromInput(object value, string settingName, ref List<Exception> exceptions)
{
    switch (value)
    {
        case null:
            exceptions.Add(...);
            return null;

        case string str:
            return GetSettingFromInput(str, settingName, ref exceptions);

        case IEnumerable enumerable:
            return GetSettingFromInput(enumerable, settingName, ref exceptions);

        default:
            exceptions.Add(...);
            return null;
    }
}

private List<string> GetSettingFromInput(string str, string settingName, ref List<Exception> exception)
{
        return GetSettingFromInput(new [] { str }, settingName, ref exception);
}

private List<string> GetSettingFromInput(IEnumerable enumerable, string settingName, ref List<Exception> exceptions)
{
    var settings = new List<string>();
    foreach (object element in enumerable)
    {
        if (element == null)
        {
            exceptions.Add(...);
            continue;
        }

        if (!(element is string strElement))
        {
            exceptions.Add(...);
            continue;
        }

        settings.Add(strElement);
    }
    return settings;
}

Note that here you don't need to use ref to pass the List<Exception>, but it helps to indicate that you intend it to be both passed in and mutated. The other possibility would be to return a tuple to lower the onus on the caller.

I don't see you use the settingName variable anywhere other than errors, but presumably that's just a question of error tracing.

severities = new List<string>();
ruleArguments = new Dictionary<string, Dictionary<string, object>>(StringComparer.OrdinalIgnoreCase);
var settingsFilePath = settings as string;
this.includeRules = new List<string>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a change like this is made, I think it may as well be a change to _includeRules instead. That's admittedly more of a style opinion, but it's consistent with other PowerShell code and a less verbose way of designating a private field.

}

if (hashtable == null)
if (hashtable is null)
Copy link
Contributor

@rjmholt rjmholt Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== is more appropriate here since it really does test referential equality. The only language I know where is is conventional for a nullity check is Python (because it doesn't really have a null, but a singleton object None that references default to).

@bergmeister
Copy link
Collaborator

@travis-c-lagrone Are you still interested in continuing the PR?

@rjmholt
Copy link
Contributor

rjmholt commented Apr 21, 2021

Closing this PR as abandoned

@rjmholt rjmholt closed this Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants