Skip to content

Commit

Permalink
Annotatively research current state of exception handling surrounding…
Browse files Browse the repository at this point in the history
… settings file load invocation
  • Loading branch information
travisclagrone committed Jun 18, 2019
1 parent e206b32 commit 3bfd45b
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 1 deletion.
12 changes: 12 additions & 0 deletions Engine/Commands/InvokeScriptAnalyzerCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ protected override void BeginProcessing()
Helper.Instance = new Helper(
SessionState.InvokeCommand,
this);
// NOTE Helper.Instance.Initialize() does *not* modify this.settings.
Helper.Instance.Initialize();

var psVersionTable = this.SessionState.PSVariable.GetValue("PSVersionTable") as Hashtable;
Expand All @@ -276,6 +277,7 @@ protected override void BeginProcessing()
Helper.Instance.SetPSVersionTable(psVersionTable);
}

// NOTE Helper.ProcessCustomRulePaths does *not* modify this.settings.
string[] rulePaths = Helper.ProcessCustomRulePaths(
customRulePath,
this.SessionState,
Expand All @@ -284,6 +286,7 @@ protected override void BeginProcessing()
if (IsFileParameterSet() && Path != null)
{
// just used to obtain the directory to use to find settings below
// NOTE ProcessPath() does *not* modify this.settings.
ProcessPath();
}

Expand All @@ -292,17 +295,24 @@ protected override void BeginProcessing()
var combIncludeDefaultRules = IncludeDefaultRules.IsPresent;
try
{
// THROW PSSASettings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws if TODO
// NOTE Microsoft.Windows.PowerShell.ScriptAnalyzer.Settings.Create(...) types the input, gets the input (if necessary), and parses it (if any).
var settingsObj = PSSASettings.Create(
// NOTHROW A null this.settings results in returning an "empty" (but not null) settingsObj without exception.
// NOTE this.settings is unmodified since start of BeginProcessing(). Thus, it is exactly the raw argument value, if any.
settings,
processedPaths == null || processedPaths.Count == 0 ? CurrentProviderLocation("FileSystem").ProviderPath : processedPaths[0],
this,
GetResolvedProviderPathFromPSPath);
// NOTE settingsObj cannot be null here since PSSASettings.Create(...) returns exactly `new Settings(settingsFound)`, which can never be null (but can throw).
if (settingsObj != null)
{
// NOTHROW UpdateSettings can throw an ArgumentNullException, but that will never happen since settingsObj is tested for non-nullity immediately above.
ScriptAnalyzer.Instance.UpdateSettings(settingsObj);

// For includeDefaultRules and RecurseCustomRulePath we override the value in the settings file by
// command line argument.
// NOTHROW InvokeScriptAnalyzerCommand.OverrideSwitchParam(bool, string)
combRecurseCustomRulePath = OverrideSwitchParam(
settingsObj.RecurseCustomRulePath,
"RecurseCustomRulePath");
Expand All @@ -315,6 +325,7 @@ protected override void BeginProcessing()
// simultaneously. But since, this was done before with IncludeRules, ExcludeRules and Severity,
// we use the same strategy for CustomRulePath. So, we take the union of CustomRulePath provided in
// the settings file and if provided on command line.
// THROW Helper.ProcessCustomRulePaths(string[], SessionState, bool) throws one of six different exceptions if a settings' custom rule path is invalid somehow (e.g. drive doesn't exit; no wildcards but item doesn't exist; provider throws a lower-level exception; etc.). See the implementation of Helper.ProcessCustomRulePaths(string[], SessionState, bool) for details.
var settingsCustomRulePath = Helper.ProcessCustomRulePaths(
settingsObj?.CustomRulePath?.ToArray(),
this.SessionState,
Expand All @@ -327,6 +338,7 @@ protected override void BeginProcessing()
}
catch
{
// NOTE Any exception in resolving, getting, parsing, updating, etc. the settings herein results in an contextless WriteWarning(Strings.SettingsNotParsable), regardless of provenance.
this.WriteWarning(String.Format(CultureInfo.CurrentCulture, Strings.SettingsNotParsable));
stopProcessing = true;
return;
Expand Down
6 changes: 6 additions & 0 deletions Engine/Helper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,12 @@ public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState s
Collection<PathInfo> pathInfo = new Collection<PathInfo>();
foreach (string rulePath in rulePaths)
{
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderNotFoundException] if path is a provider-qualified path and the specified provider does not exist.
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.DriveNotFoundException] if path is a drive-qualified path and the specified drive does not exist.
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderInvocationException] if the provider throws an exception when its MakePath gets called.
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.NotSupportedException] if the provider does not support multiple items.
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.InvalidOperationException] the home location for the provider is not set and path starts with a "~".
// THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ItemNotFoundException] if path does not contain wildcard characters and could not be found.
Collection<PathInfo> pathInfosForRulePath = sessionState.Path.GetResolvedPSPathFromPSPath(rulePath);
if (null != pathInfosForRulePath)
{
Expand Down
21 changes: 20 additions & 1 deletion Engine/Settings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ public Settings(object settings, Func<string, string> presetResolver)
if (File.Exists(settingsFilePath))
{
filePath = settingsFilePath;
// QUESTION When does parseSettingsFile(string) throw?
parseSettingsFile(settingsFilePath);
}
else
{
// THROW ArgumentException(Strings.InvalidPath) if the resolution of the settings argument via GetResolvedProviderPathFromPSPathDelegate is non-null but not an existing file. (e.g. it may be a directory)
throw new ArgumentException(
String.Format(
CultureInfo.CurrentCulture,
Expand All @@ -89,10 +91,12 @@ public Settings(object settings, Func<string, string> presetResolver)
var settingsHashtable = settings as Hashtable;
if (settingsHashtable != null)
{
// QUESTION When does parseSettingsHashtable(Hashtable) throw?
parseSettingsHashtable(settingsHashtable);
}
else
{
// THROW ArgumentException(Strings.SettingsInvalidType) if settings is not convertible (`as` keyword) to Hashtable.
throw new ArgumentException(Strings.SettingsInvalidType);
}
}
Expand Down Expand Up @@ -179,11 +183,12 @@ public static string GetSettingPresetFilePath(string settingPreset)
/// <param name="outputWriter">An output writer.</param>
/// <param name="getResolvedProviderPathFromPSPathDelegate">The GetResolvedProviderPathFromPSPath method from PSCmdlet to resolve relative path including wildcard support.</param>
/// <returns>An object of Settings type.</returns>
// THROW Settings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws only because of the contained invocation to the Settings constructor, which does throw.
internal static Settings Create(object settingsObj, string cwd, IOutputWriter outputWriter,
PathResolver.GetResolvedProviderPathFromPSPath<string, ProviderInfo, Collection<string>> getResolvedProviderPathFromPSPathDelegate)
{
object settingsFound;
var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound);
var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound); // NOTHROW

switch (settingsMode)
{
Expand All @@ -205,6 +210,7 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou
var userProvidedSettingsString = settingsFound.ToString();
try
{
// THROW ..Single() throws [System.InvalidOperationException] if the settings path does not resolve to exactly one item. (more or less)
var resolvedPath = getResolvedProviderPathFromPSPathDelegate(userProvidedSettingsString, out ProviderInfo providerInfo).Single();
settingsFound = resolvedPath;
outputWriter?.WriteVerbose(
Expand All @@ -213,8 +219,11 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou
Strings.SettingsUsingFile,
resolvedPath));
}
// NOTE This catch block effectively makes *any* exception resulting from resolving the settings file path reduce to the case of no settings (by way of null settingsFound). Only a WriteVerbose(Strings.SettingsCannotFindFile) identifies what happened.
catch
{
// TODO Upgrade WriteVerbose(Strings.SettingsCannotFindFile) to WriteWarning(Strings.SettingsCannotFindFile).
// TODO Perform a ShouldContinue (?) confirmation check in the event that an exception occurred while resolving the settings file path.
outputWriter?.WriteVerbose(
String.Format(
CultureInfo.CurrentCulture,
Expand All @@ -238,6 +247,7 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou
return null;
}

// THROW new Settings(object)
return new Settings(settingsFound);
}

Expand Down Expand Up @@ -456,16 +466,19 @@ private void parseSettingsHashtable(Hashtable settingsHashtable)
}
}

// NOTE parseSettingsFile concludes by calling parseSettingsHashtable if it (parseSettingsFile) is successful.
private void parseSettingsFile(string settingsFilePath)
{
Token[] parserTokens = null;
ParseError[] parserErrors = null;
// QUESTION Can Parser.ParseFile throw?
Ast profileAst = Parser.ParseFile(settingsFilePath, out parserTokens, out parserErrors);
IEnumerable<Ast> hashTableAsts = profileAst.FindAll(item => item is HashtableAst, false);

// no hashtable, raise warning
if (hashTableAsts.Count() == 0)
{
// THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if no hashTableAst is detected in settings file.
throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Strings.InvalidProfile, settingsFilePath));
}

Expand All @@ -475,22 +488,26 @@ private void parseSettingsFile(string settingsFilePath)
{
// ideally we should use HashtableAst.SafeGetValue() but since
// it is not available on PSv3, we resort to our own narrow implementation.
// QUESTION When does GetSafeValueFromHashtableAst(hashTableAst) throw?
hashtable = GetSafeValueFromHashtableAst(hashTableAst);
}
catch (InvalidOperationException e)
{
// THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if GetSafeValueFromHashtableAst(hashTableAst) throws an InvalidOperationException.
throw new ArgumentException(Strings.InvalidProfile, e);
}

if (hashtable == null)
{
// THROW parseSettingsFile throws ArgumentException if GetSafeValueFromHashtableAst(hashTableAst) returns null.
throw new ArgumentException(
String.Format(
CultureInfo.CurrentCulture,
Strings.InvalidProfile,
settingsFilePath));
}

// QUESTION When does parseSettingsHashtable(Hashtable) throw?
parseSettingsHashtable(hashtable);
}

Expand Down Expand Up @@ -686,6 +703,7 @@ private static bool IsBuiltinSettingPreset(object settingPreset)
return false;
}

// NOTHROW FindSettingsMode(object, string, out object)
internal static SettingsMode FindSettingsMode(object settings, string path, out object settingsFound)
{
var settingsMode = SettingsMode.None;
Expand All @@ -706,6 +724,7 @@ internal static SettingsMode FindSettingsMode(object settings, string path, out
{
// if settings are not provided explicitly, look for it in the given path
// check if pssasettings.psd1 exists
// COMBAK Refactor the magic string literal "PSScriptAnalyzerSettings.psd1" to a public const field on the class. (bare minimum... although will also help open possibility for user-configurable default name)
var settingsFilename = "PSScriptAnalyzerSettings.psd1";
var settingsFilePath = Path.Combine(directory, settingsFilename);
settingsFound = settingsFilePath;
Expand Down

0 comments on commit 3bfd45b

Please sign in to comment.