From 5cd8a3afd10f8f87c5ab9c07f02bba63afa5e960 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 25 Apr 2016 17:45:33 -0700 Subject: [PATCH 1/3] Add suggested correction to MissingModuleManifestField rule --- Rules/MissingModuleManifestField.cs | 51 +++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index 0acd85278..f41515b7c 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -17,6 +17,7 @@ using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; using System.ComponentModel.Composition; using System.Globalization; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -46,9 +47,17 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (Helper.IsMissingManifestMemberException(errorRecord)) { - System.Diagnostics.Debug.Assert(errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), Strings.NullErrorMessage); - yield return - new DiagnosticRecord(errorRecord.Exception.Message, ast.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + System.Diagnostics.Debug.Assert( + errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), + Strings.NullErrorMessage); + var hashTableAsts = ast.Find(x => x is HashtableAst, false); + yield return new DiagnosticRecord( + errorRecord.Exception.Message, + ast.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suggestedCorrections:GetCorrectionExtent(hashTableAsts as HashtableAst)); } } @@ -56,6 +65,42 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } + + + /// + /// Gets the correction extent + /// + /// + /// A List of CorrectionExtent + private List GetCorrectionExtent(HashtableAst ast) + { + // we assume the extent begins with "@{" + if (ast.Extent.Text.IndexOf("@{") != 0) + { + return null; + } + + var correctionExtents = new List(); + string moduleVersionText = @" +# Version number of this module. +ModuleVersion = '1.0.0.0'"; + int startLineNumber = ast.Extent.StartLineNumber; + int startColumnNumber = ast.Extent.StartColumnNumber + 2; // 2 for "@{", + var correctionText = new StringBuilder(); + correctionText.AppendLine(); + correctionText.Append(moduleVersionText); + correctionText.AppendLine(); + correctionText.AppendLine(); + var correctionExtent = new CorrectionExtent( + startLineNumber, + startLineNumber, + startColumnNumber, + startColumnNumber + 1, + correctionText.ToString(), + ast.Extent.File); + correctionExtents.Add(correctionExtent); + return correctionExtents; + } /// /// GetName: Retrieves the name of this rule. From b943634d0fec588b78031430bc61fe95d8f1c08f Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 26 Apr 2016 10:04:09 -0700 Subject: [PATCH 2/3] Add tests for MissingModuleManifestFields * Modifies the implmentation to add description and other minor changes * Adds test cases to verify the correction extent of the rule --- Rules/MissingModuleManifestField.cs | 28 +++++++++++------- Rules/Strings.Designer.cs | 9 ++++++ Rules/Strings.resx | 3 ++ ...OrMissingRequiredFieldInManifest.tests.ps1 | 29 +++++++++++++++++-- 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index f41515b7c..c1fa770ce 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -81,23 +81,31 @@ private List GetCorrectionExtent(HashtableAst ast) } var correctionExtents = new List(); - string moduleVersionText = @" -# Version number of this module. -ModuleVersion = '1.0.0.0'"; + string fieldName = "ModuleVersion"; + string fieldValue = "1.0.0.0"; int startLineNumber = ast.Extent.StartLineNumber; int startColumnNumber = ast.Extent.StartColumnNumber + 2; // 2 for "@{", - var correctionText = new StringBuilder(); - correctionText.AppendLine(); - correctionText.Append(moduleVersionText); - correctionText.AppendLine(); - correctionText.AppendLine(); + string description = string.Format( + CultureInfo.CurrentCulture, + Strings.MissingModuleManifestFieldCorrectionDescription, + fieldName, + fieldValue); + var correctionTextTemplate = @" +# Version number of this module. +{0} = {1} +"; + var correctionText = string.Format( + correctionTextTemplate, + fieldName, + fieldValue); var correctionExtent = new CorrectionExtent( startLineNumber, startLineNumber, startColumnNumber, startColumnNumber + 1, - correctionText.ToString(), - ast.Extent.File); + correctionText, + ast.Extent.File, + description); correctionExtents.Add(correctionExtent); return correctionExtents; } diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index ee3aaebe2..e22664439 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1113,6 +1113,15 @@ internal static string MissingModuleManifestFieldCommonName { } } + /// + /// Looks up a localized string similar to Add {0} = {1} to the module manifest. + /// + internal static string MissingModuleManifestFieldCorrectionDescription { + get { + return ResourceManager.GetString("MissingModuleManifestFieldCorrectionDescription", resourceCulture); + } + } + /// /// Looks up a localized string similar to Some fields of the module manifest (such as ModuleVersion) are required.. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index a07448d27..25deb4125 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -816,6 +816,9 @@ Set {0} type to SecureString + + Add {0} = {1} to the module manifest + Replace {0} with {1} diff --git a/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 b/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 index 8f828d9ac..3a2a902e3 100644 --- a/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 +++ b/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 @@ -2,10 +2,19 @@ $missingMessage = "The member 'ModuleVersion' is not present in the module manifest." $missingName = "PSMissingModuleManifestField" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\TestBadModule\TestBadModule.psd1 | Where-Object {$_.RuleName -eq $missingName} +$violationFilepath = Join-Path $directory "TestBadModule\TestBadModule.psd1" +$violations = Invoke-ScriptAnalyzer $violationFilepath | Where-Object {$_.RuleName -eq $missingName} $noViolations = Invoke-ScriptAnalyzer $directory\TestGoodModule\TestGoodModule.psd1 | Where-Object {$_.RuleName -eq $missingName} Describe "MissingRequiredFieldModuleManifest" { + BeforeAll { + Import-Module (Join-Path $directory "PSScriptAnalyzerTestHelper.psm1") + } + + AfterAll{ + Remove-Module PSScriptAnalyzerTestHelper + } + Context "When there are violations" { It "has 1 missing required field module manifest violation" { $violations.Count | Should Be 1 @@ -14,7 +23,23 @@ Describe "MissingRequiredFieldModuleManifest" { It "has the correct description message" { $violations.Message | Should Match $missingMessage } - } + + $expectedCorrections = 1 + It "has $expectedCorrections suggested corrections" { + $violations.SuggestedCorrections.Count | Should Be 1 + } + + + It "has the right suggested correction" { + $expectedText = @' + +# Version number of this module. +ModuleVersion = 1.0.0.0 +'@ + $violations[0].SuggestedCorrections[0].Text | Should Match $expectedText + Get-ExtentText $violations[0].SuggestedCorrections[0] $violationFilepath | Should Match "" + } +} Context "When there are no violations" { It "returns no violations" { From 5da7ca2bfa8d0b7d9ac88c78b6597b8ef1c95791 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 29 Apr 2016 17:52:03 -0700 Subject: [PATCH 3/3] Modify MissingModuleManifest correction behavior * Insert ModuleVersion field at the end of the hashtable instead of the beginning * Fix MissingModuleManifest test case --- Rules/MissingModuleManifestField.cs | 45 ++++++++++++++----- ...OrMissingRequiredFieldInManifest.tests.ps1 | 9 ++-- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/Rules/MissingModuleManifestField.cs b/Rules/MissingModuleManifestField.cs index c1fa770ce..13017aa0b 100644 --- a/Rules/MissingModuleManifestField.cs +++ b/Rules/MissingModuleManifestField.cs @@ -50,14 +50,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) System.Diagnostics.Debug.Assert( errorRecord.Exception != null && !String.IsNullOrWhiteSpace(errorRecord.Exception.Message), Strings.NullErrorMessage); - var hashTableAsts = ast.Find(x => x is HashtableAst, false); + var hashTableAst = ast.Find(x => x is HashtableAst, false); yield return new DiagnosticRecord( errorRecord.Exception.Message, - ast.Extent, + hashTableAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, - suggestedCorrections:GetCorrectionExtent(hashTableAsts as HashtableAst)); + suggestedCorrections:GetCorrectionExtent(hashTableAst as HashtableAst)); } } @@ -66,7 +66,6 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } - /// /// Gets the correction extent /// @@ -74,17 +73,39 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// A List of CorrectionExtent private List GetCorrectionExtent(HashtableAst ast) { - // we assume the extent begins with "@{" - if (ast.Extent.Text.IndexOf("@{") != 0) - { - return null; + int startLineNumber; + int startColumnNumber; + + // for empty hashtable insert after after "@{" + if (ast.KeyValuePairs.Count == 0) + { + // check if ast starts with "@{" + if (ast.Extent.Text.IndexOf("@{") != 0) + { + return null; + } + startLineNumber = ast.Extent.StartLineNumber; + startColumnNumber = ast.Extent.StartColumnNumber + 2; // 2 for "@{", + } + else // for non-empty hashtable insert after the last element + { + int maxLine = 0; + int lastCol = 0; + foreach (var keyVal in ast.KeyValuePairs) + { + if (keyVal.Item2.Extent.EndLineNumber > maxLine) + { + maxLine = keyVal.Item2.Extent.EndLineNumber; + lastCol = keyVal.Item2.Extent.EndColumnNumber; + } + } + startLineNumber = maxLine; + startColumnNumber = lastCol; } var correctionExtents = new List(); string fieldName = "ModuleVersion"; string fieldValue = "1.0.0.0"; - int startLineNumber = ast.Extent.StartLineNumber; - int startColumnNumber = ast.Extent.StartColumnNumber + 2; // 2 for "@{", string description = string.Format( CultureInfo.CurrentCulture, Strings.MissingModuleManifestFieldCorrectionDescription, @@ -92,7 +113,7 @@ private List GetCorrectionExtent(HashtableAst ast) fieldValue); var correctionTextTemplate = @" # Version number of this module. -{0} = {1} +{0} = '{1}' "; var correctionText = string.Format( correctionTextTemplate, @@ -102,7 +123,7 @@ private List GetCorrectionExtent(HashtableAst ast) startLineNumber, startLineNumber, startColumnNumber, - startColumnNumber + 1, + startColumnNumber, correctionText, ast.Extent.File, description); diff --git a/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 b/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 index 3a2a902e3..5a686eed1 100644 --- a/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 +++ b/Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1 @@ -24,17 +24,16 @@ Describe "MissingRequiredFieldModuleManifest" { $violations.Message | Should Match $missingMessage } - $expectedCorrections = 1 - It "has $expectedCorrections suggested corrections" { - $violations.SuggestedCorrections.Count | Should Be 1 + $numExpectedCorrections = 1 + It "has $numExpectedCorrections suggested corrections" { + $violations.SuggestedCorrections.Count | Should Be $numExpectedCorrections } It "has the right suggested correction" { $expectedText = @' - # Version number of this module. -ModuleVersion = 1.0.0.0 +ModuleVersion = '1.0.0.0' '@ $violations[0].SuggestedCorrections[0].Text | Should Match $expectedText Get-ExtentText $violations[0].SuggestedCorrections[0] $violationFilepath | Should Match ""