From 92066ecdf5f68394d2892de19fc7b0378104c494 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 11 May 2017 15:37:16 +0300 Subject: [PATCH 01/20] Add dynamically generated set in ValidateSetAttribute for binary cmdlets --- .../engine/Attributes.cs | 38 ++++++++ .../Scripting.Classes.Attributes.Tests.ps1 | 95 +++++++++++++++++++ 2 files changed, 133 insertions(+) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 0961b26e8ce..1ce17fee62f 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -10,6 +10,7 @@ using System.Diagnostics.CodeAnalysis; using System.Management.Automation.Language; using System.Runtime.CompilerServices; +using System.Linq; namespace System.Management.Automation.Internal { @@ -1455,6 +1456,43 @@ public ValidateSetAttribute(params string[] validValues) _validValues = validValues; } + + /// + /// Initializes a new instance of the ValidateSetAttribute class + /// + /// list of valid values + /// for null arguments + /// for invalid arguments + public ValidateSetAttribute(Type type) + { + IValidateSetValuesGenerator validValuesGenerator; + if (typeof(IValidateSetValuesGenerator).IsAssignableFrom(type)) + { + validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(type); + } + else + { + throw PSTraceSource.NewArgumentNullException("validValues"); + } + + _validValues = validValuesGenerator.GetValidValues().ToArray(); + + if (_validValues.Length == 0) + { + throw PSTraceSource.NewArgumentOutOfRangeException("IValidateSetValuesGenerator", type); + } + } + } + + /// + /// Allows dynamically generate set of values for ValidateSetAttribute. + /// + public interface IValidateSetValuesGenerator + { + /// + /// Get a valid values. + /// + IEnumerable GetValidValues(); } #region Allow diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index e29e4676477..c9df8fd986e 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -1,3 +1,5 @@ +Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1 + Describe 'Attributes Test' -Tags "CI" { BeforeAll { @@ -216,3 +218,96 @@ Describe 'Type resolution with attributes' -Tag "CI" { } } } + +Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { + Context 'C# test' { + + BeforeAll { + $a=@' + using System; + using System.Management.Automation; + using System.Collections.Generic; + + namespace Test.Language { + + [Cmdlet(VerbsCommon.Get, "TestValidateSet1")] + public class TestValidateSetCommand1 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParam1))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } + + [Cmdlet(VerbsCommon.Get, "TestValidateSet2")] + public class TestValidateSetCommand2 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(PSCmdlet))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } + + [Cmdlet(VerbsCommon.Get, "TestValidateSet3")] + public class TestValidateSetCommand3 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParam3))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } + + + /// Implement of test IValidateSetValuesGenerator + public class GenValuesForParam1 : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() + { + var testValues = new string[] {"Test1","TestString","Test2"}; + foreach (var value in testValues) + { + yield return value; + } + } + } + + /// Implement of test IValidateSetValuesGenerator to return Null +#pragma warning disable 0162 + public class GenValuesForParam3 : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() + { + if (false) yield return "TestString"; + } + } + } +'@ + + Add-Type -TypeDefinition $a -PassThru | % {$_.assembly} | Import-module -Force + } + + It 'Dynamically generated set work' { + Get-TestValidateSet1 -Param1 "TestString" -ErrorAction SilentlyContinue | Should BeExactly "TestString" + } + + It 'Throw if IValidateSetValuesGenerator is not implemented' { + { Get-TestValidateSet2 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "ArgumentNull" + } + + It 'Throw if IValidateSetValuesGenerator returns a null' { + { Get-TestValidateSet3 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "ArgumentOutOfRange" + } + } +} From 78c8bdabd7b6a6ec6ec1b1006b34c4c3114e8494 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Tue, 16 May 2017 18:52:17 +0300 Subject: [PATCH 02/20] Remove caching valid values and unneeded test --- .../engine/Attributes.cs | 27 +++++++----------- .../Scripting.Classes.Attributes.Tests.ps1 | 28 +------------------ 2 files changed, 11 insertions(+), 44 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 1ce17fee62f..20ebf6ce4e0 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1360,7 +1360,7 @@ public ValidateCountAttribute(int minLength, int maxLength) [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)] public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute { - private string[] _validValues; + private IEnumerable _validValues; /// /// Gets or sets the custom error message that is displayed to the user @@ -1387,7 +1387,7 @@ public IList ValidValues { get { - return _validValues; + return _validValues.ToList(); } } @@ -1410,16 +1410,13 @@ protected override void ValidateElement(object element) } string objString = element.ToString(); - for (int setIndex = 0; setIndex < _validValues.Length; setIndex++) + foreach (string setString in _validValues) { - string setString = _validValues[setIndex]; - if (CultureInfo.InvariantCulture. CompareInfo.Compare(setString, objString, IgnoreCase ? CompareOptions.IgnoreCase : CompareOptions.None) == 0) - { return; } @@ -1460,27 +1457,23 @@ public ValidateSetAttribute(params string[] validValues) /// /// Initializes a new instance of the ValidateSetAttribute class /// - /// list of valid values + /// type that implements the 'IValidateSetValuesGenerator' interface /// for null arguments /// for invalid arguments - public ValidateSetAttribute(Type type) + public ValidateSetAttribute(Type valuesGeneratorType) { IValidateSetValuesGenerator validValuesGenerator; - if (typeof(IValidateSetValuesGenerator).IsAssignableFrom(type)) + + if (typeof(IValidateSetValuesGenerator).IsAssignableFrom(valuesGeneratorType)) { - validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(type); + validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(valuesGeneratorType); } else { - throw PSTraceSource.NewArgumentNullException("validValues"); + throw PSTraceSource.NewArgumentException("valuesGeneratorType"); } - _validValues = validValuesGenerator.GetValidValues().ToArray(); - - if (_validValues.Length == 0) - { - throw PSTraceSource.NewArgumentOutOfRangeException("IValidateSetValuesGenerator", type); - } + _validValues = validValuesGenerator.GetValidValues(); } } diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index c9df8fd986e..cc881627397 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -256,18 +256,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } - [Cmdlet(VerbsCommon.Get, "TestValidateSet3")] - public class TestValidateSetCommand3 : PSCmdlet - { - [Parameter] - [ValidateSet(typeof(GenValuesForParam3))] - public string Param1; - - protected override void EndProcessing() - { - WriteObject(Param1); - } - } /// Implement of test IValidateSetValuesGenerator @@ -282,16 +270,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } } - - /// Implement of test IValidateSetValuesGenerator to return Null -#pragma warning disable 0162 - public class GenValuesForParam3 : IValidateSetValuesGenerator - { - public IEnumerable GetValidValues() - { - if (false) yield return "TestString"; - } - } } '@ @@ -303,11 +281,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } It 'Throw if IValidateSetValuesGenerator is not implemented' { - { Get-TestValidateSet2 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "ArgumentNull" - } - - It 'Throw if IValidateSetValuesGenerator returns a null' { - { Get-TestValidateSet3 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "ArgumentOutOfRange" + { Get-TestValidateSet2 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" } } } From 84145aa882e6f8c597d4f1a2d33a3cbb33503853 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Wed, 17 May 2017 14:03:09 +0300 Subject: [PATCH 03/20] Add "powershell" test --- .../Scripting.Classes.Attributes.Tests.ps1 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index cc881627397..f6723850da5 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -274,6 +274,23 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { '@ Add-Type -TypeDefinition $a -PassThru | % {$_.assembly} | Import-module -Force + + class GenValuesForParam2 : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { return [string[]]("Test1","TestString","Test2") } + } + + function Get-TestValidateSet3 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParam2])] + $Param1 + ) + + $Param1 + } + } It 'Dynamically generated set work' { From 788f9102fd2dcf53ae30143eff9a9b2918eed900 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 19 May 2017 17:16:01 +0300 Subject: [PATCH 04/20] Dynamic binding for ValidateSetAttribute --- src/System.Management.Automation/engine/parser/Compiler.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index 34c0828daa3..d3253034a66 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -739,7 +739,6 @@ static Compiler() s_builtinAttributeGenerator.Add(typeof(ParameterAttribute), NewParameterAttribute); s_builtinAttributeGenerator.Add(typeof(OutputTypeAttribute), NewOutputTypeAttribute); s_builtinAttributeGenerator.Add(typeof(AliasAttribute), NewAliasAttribute); - s_builtinAttributeGenerator.Add(typeof(ValidateSetAttribute), NewValidateSetAttribute); s_builtinAttributeGenerator.Add(typeof(DebuggerHiddenAttribute), NewDebuggerHiddenAttribute); s_builtinAttributeGenerator.Add(typeof(ValidateNotNullAttribute), NewValidateNotNullAttribute); s_builtinAttributeGenerator.Add(typeof(ValidateNotNullOrEmptyAttribute), NewValidateNotNullOrEmptyAttribute); From 021f27c9a4e46d02e6d9c9fcd8f982807c7143a7 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Mon, 22 May 2017 18:06:22 +0300 Subject: [PATCH 05/20] Add caching valid values and tests --- .../engine/Attributes.cs | 46 +++-- .../Scripting.Classes.Attributes.Tests.ps1 | 192 +++++++++++++----- 2 files changed, 174 insertions(+), 64 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 20ebf6ce4e0..376aaa7d533 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1360,7 +1360,11 @@ public ValidateCountAttribute(int minLength, int maxLength) [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)] public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute { - private IEnumerable _validValues; + /// We can use either static '_validValues' or dynamic '_validValuesGenerator' valid values list + private string[] _validValues; + private Type validValuesGeneratorType = null; + private IValidateSetValuesGenerator validValuesGenerator = null; + private DateTime cacheExpiration = DateTime.MinValue; /// /// Gets or sets the custom error message that is displayed to the user @@ -1380,6 +1384,11 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute /// public bool IgnoreCase { get; set; } = true; + /// + /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. + /// + public int CacheExpiration { get; set; } = 0; + /// /// Gets the values in the set /// @@ -1387,7 +1396,20 @@ public IList ValidValues { get { - return _validValues.ToList(); + if (validValuesGeneratorType != null) + { + var currentTime = DateTime.Now; + if (DateTime.Compare(cacheExpiration, currentTime) < 0) + { + cacheExpiration = currentTime.AddSeconds(CacheExpiration); + if (validValuesGenerator == null) + { + validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); + } + _validValues = validValuesGenerator.GetValidValues().ToArray(); + } + } + return _validValues; } } @@ -1410,7 +1432,7 @@ protected override void ValidateElement(object element) } string objString = element.ToString(); - foreach (string setString in _validValues) + foreach (string setString in ValidValues) { if (CultureInfo.InvariantCulture. CompareInfo.Compare(setString, objString, @@ -1455,25 +1477,19 @@ public ValidateSetAttribute(params string[] validValues) } /// - /// Initializes a new instance of the ValidateSetAttribute class + /// Initializes a new instance of the ValidateSetAttribute class. + /// Valid values is returned dynamically from a custom class implementing 'IValidateSetValuesGenerator' interface. /// - /// type that implements the 'IValidateSetValuesGenerator' interface - /// for null arguments - /// for invalid arguments + /// class that implements the 'IValidateSetValuesGenerator' interface + /// for null arguments public ValidateSetAttribute(Type valuesGeneratorType) { - IValidateSetValuesGenerator validValuesGenerator; - - if (typeof(IValidateSetValuesGenerator).IsAssignableFrom(valuesGeneratorType)) - { - validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(valuesGeneratorType); - } - else + if (!typeof(IValidateSetValuesGenerator).IsAssignableFrom(valuesGeneratorType)) { throw PSTraceSource.NewArgumentException("valuesGeneratorType"); } - _validValues = validValuesGenerator.GetValidValues(); + validValuesGeneratorType = valuesGeneratorType; } } diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index f6723850da5..250edfe2533 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -220,85 +220,179 @@ Describe 'Type resolution with attributes' -Tag "CI" { } Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { - Context 'C# test' { - BeforeAll { - $a=@' - using System; - using System.Management.Automation; - using System.Collections.Generic; + BeforeAll { + $a=@' + using System; + using System.Management.Automation; + using System.Collections.Generic; + + namespace Test.Language { - namespace Test.Language { + [Cmdlet(VerbsCommon.Get, "TestValidateSet1")] + public class TestValidateSetCommand1 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(PSCmdlet))] + public string Param1; - [Cmdlet(VerbsCommon.Get, "TestValidateSet1")] - public class TestValidateSetCommand1 : PSCmdlet + protected override void EndProcessing() { - [Parameter] - [ValidateSet(typeof(GenValuesForParam1))] - public string Param1; + WriteObject(Param1); + } + } - protected override void EndProcessing() - { - WriteObject(Param1); - } + [Cmdlet(VerbsCommon.Get, "TestValidateSet2")] + public class TestValidateSetCommand2 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParam1))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); } + } - [Cmdlet(VerbsCommon.Get, "TestValidateSet2")] - public class TestValidateSetCommand2 : PSCmdlet + [Cmdlet(VerbsCommon.Get, "TestValidateSet3")] + public class TestValidateSetCommand3 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParam1Cache), CacheExpiration = 2)] + public string Param1; + + protected override void EndProcessing() { - [Parameter] - [ValidateSet(typeof(PSCmdlet))] - public string Param1; + WriteObject(Param1); + } + } + - protected override void EndProcessing() + + /// Implement of test IValidateSetValuesGenerator + public class GenValuesForParam1 : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() + { + var testValues = new string[] {"Test1","TestString1","Test2"}; + foreach (var value in testValues) { - WriteObject(Param1); + yield return value; } } + } - - - /// Implement of test IValidateSetValuesGenerator - public class GenValuesForParam1 : IValidateSetValuesGenerator + public class GenValuesForParam1Cache : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() { - public IEnumerable GetValidValues() + var testValues1 = new string[] {"Test1","TestString1","Test2"}; + var testValues2 = new string[] {"Test1","TestString2","Test2"}; + string[] testValues; + + var currentTime = DateTime.Now; + if (DateTime.Compare(_cacheTime, currentTime) <= 0) + { + testValues = testValues1; + //_cacheTime = currentTime.AddSeconds(2); + } + else + { + testValues = testValues2; + } + foreach (var value in testValues) { - var testValues = new string[] {"Test1","TestString","Test2"}; - foreach (var value in testValues) - { - yield return value; - } + yield return value; } } + private static DateTime _cacheTime = DateTime.Now.AddSeconds(2); } + } '@ - Add-Type -TypeDefinition $a -PassThru | % {$_.assembly} | Import-module -Force + Add-Type -TypeDefinition $a -PassThru | % {$_.assembly} | Import-module -Force + + class GenValuesForParam : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { - class GenValuesForParam2 : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { return [string[]]("Test1","TestString","Test2") } + return [string[]]("Test1","TestString1","Test2") } - function Get-TestValidateSet3 - { - [CmdletBinding()] - Param - ( - [ValidateSet([GenValuesForParam2])] - $Param1 - ) + [DateTime] $cacheTime; + } - $Param1 + # Return '$testValues2' and after 2 seconds after first use return another array '$testValues1'. + class GenValuesForParamCache : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + + $testValues1 = "Test1","TestString1","Test2" + $testValues2 = "Test1","TestString2","Test2" + + $currentTime = [DateTime]::Now + if ([DateTime]::Compare([GenValuesForParamCache]::cacheTime, $currentTime) -le 0) + { + $testValues = $testValues1; + } + else + { + $testValues = $testValues2; + } + return [string[]]$testValues } + static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); } - It 'Dynamically generated set work' { - Get-TestValidateSet1 -Param1 "TestString" -ErrorAction SilentlyContinue | Should BeExactly "TestString" + function Get-TestValidateSet4 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParam])] + $Param1 + ) + + $Param1 } - It 'Throw if IValidateSetValuesGenerator is not implemented' { - { Get-TestValidateSet2 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" + function Get-TestValidateSet5 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParamCache], CacheExpiration = 2)] + $Param1 + ) + + $Param1 } + + } + + It 'Throw if IValidateSetValuesGenerator is not implemented' { + { Get-TestValidateSet1 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" + } + + It 'Dynamically generated set work in C#' { + Get-TestValidateSet2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } + + It 'Dynamically generated set work in C# with cache expiration' { + Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + Start-Sleep -Seconds 2 + Get-TestValidateSet3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } + + It 'Dynamically generated set work in PowerShell script' { + Get-TestValidateSet4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } + + It 'Dynamically generated set work in PowerShell script with cache expiration' { + Get-TestValidateSet5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + Get-TestValidateSet5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + Start-Sleep -Seconds 2 + Get-TestValidateSet5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" } } From e45612f3041c58b64857ecc164cb914b31a31205 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Wed, 24 May 2017 15:03:42 +0300 Subject: [PATCH 06/20] Add global cache for valid values --- .../engine/Attributes.cs | 63 +++- .../resources/Metadata.resx | 3 + .../Scripting.Classes.Attributes.Tests.ps1 | 343 ++++++++++++++---- 3 files changed, 326 insertions(+), 83 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 376aaa7d533..d84671e17a5 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -4,6 +4,7 @@ using System.Collections; using System.Collections.Generic; +using System.Collections.Concurrent; using System.Text.RegularExpressions; using System.Globalization; using System.Management.Automation.Internal; @@ -1360,11 +1361,32 @@ public ValidateCountAttribute(int minLength, int maxLength) [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)] public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute { - /// We can use either static '_validValues' or dynamic '_validValuesGenerator' valid values list + // We can use either static '_validValues' + // or dynamic valid values list generated by instance of 'validValuesGeneratorType'. private string[] _validValues; + + // The valid values cache works across 'ValidateSetAttribute' instances. + // By default 'CacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). + private static ConcurrentDictionary validValuesCache = new ConcurrentDictionary(); + private class ValidValuesCacheEntry + { + public IValidateSetValuesGenerator validValuesGenerator = null; + public string[] validValues; + public DateTime cacheEntryCreationTime = DateTime.MinValue; + + public ValidValuesCacheEntry(IValidateSetValuesGenerator validValuesGenerator, string[] validValues, DateTime cacheEntryCreationTime) + { + this.validValuesGenerator = validValuesGenerator; + this.validValues = validValues; + this.cacheEntryCreationTime = cacheEntryCreationTime; + } + } private Type validValuesGeneratorType = null; - private IValidateSetValuesGenerator validValuesGenerator = null; - private DateTime cacheExpiration = DateTime.MinValue; + + /// + /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. + /// + public int CacheExpiration { get; set; } = 0; /// /// Gets or sets the custom error message that is displayed to the user @@ -1384,11 +1406,6 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute /// public bool IgnoreCase { get; set; } = true; - /// - /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. - /// - public int CacheExpiration { get; set; } = 0; - /// /// Gets the values in the set /// @@ -1398,16 +1415,34 @@ public IList ValidValues { if (validValuesGeneratorType != null) { - var currentTime = DateTime.Now; - if (DateTime.Compare(cacheExpiration, currentTime) < 0) + ValidValuesCacheEntry validValuesCacheEntry; + if (validValuesCache.TryGetValue(validValuesGeneratorType, out validValuesCacheEntry)) { - cacheExpiration = currentTime.AddSeconds(CacheExpiration); - if (validValuesGenerator == null) + var currentTime = DateTime.Now; + if (DateTime.Compare(validValuesCacheEntry.cacheEntryCreationTime.AddSeconds(CacheExpiration), currentTime) < 0) { - validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); + // Update expired Entry. + validValuesCacheEntry.validValues = validValuesCacheEntry.validValuesGenerator.GetValidValues()?.ToArray(); + validValuesCacheEntry.cacheEntryCreationTime = currentTime; + validValuesCache[validValuesGeneratorType] = validValuesCacheEntry; } - _validValues = validValuesGenerator.GetValidValues().ToArray(); } + else + { + var validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); + var validValues = validValuesGenerator.GetValidValues()?.ToArray(); + var cacheEntryCreationTime = DateTime.Now; + validValuesCacheEntry = new ValidValuesCacheEntry(validValuesGenerator, validValues, cacheEntryCreationTime); + validValuesCache.TryAdd(validValuesGeneratorType, validValuesCacheEntry); + } + if (validValuesCacheEntry.validValues == null) + { + throw new ValidationMetadataException( + "ValidateSetGeneratedValidValuesListIsEmpty", + null, + Metadata.ValidateSetGeneratedValidValuesListIsEmpty); + } + _validValues = validValuesCacheEntry.validValues; } return _validValues; } diff --git a/src/System.Management.Automation/resources/Metadata.resx b/src/System.Management.Automation/resources/Metadata.resx index 14c10bbe25a..a6144717d6c 100644 --- a/src/System.Management.Automation/resources/Metadata.resx +++ b/src/System.Management.Automation/resources/Metadata.resx @@ -168,6 +168,9 @@ The argument "{0}" does not belong to the set "{1}" specified by the ValidateSet attribute. Supply an argument that is in the set and then try the command again. + + Valid values generator return a null value. + "{0}" failed on property "{1}" {2} diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index 250edfe2533..a3f94300ea0 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -1,5 +1,3 @@ -Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1 - Describe 'Attributes Test' -Tags "CI" { BeforeAll { @@ -221,19 +219,34 @@ Describe 'Type resolution with attributes' -Tag "CI" { Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { - BeforeAll { - $a=@' + Context 'C# tests' { + + BeforeAll { + $a=@' using System; using System.Management.Automation; using System.Collections.Generic; namespace Test.Language { + [Cmdlet(VerbsCommon.Get, "TestValidateSet0")] + public class TestValidateSetCommand0 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(PSCmdlet))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } + [Cmdlet(VerbsCommon.Get, "TestValidateSet1")] public class TestValidateSetCommand1 : PSCmdlet { [Parameter] - [ValidateSet(typeof(PSCmdlet))] + [ValidateSet(typeof(GenValuesForParamCache1), CacheExpiration = 2)] public string Param1; protected override void EndProcessing() @@ -246,7 +259,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { public class TestValidateSetCommand2 : PSCmdlet { [Parameter] - [ValidateSet(typeof(GenValuesForParam1))] + [ValidateSet(typeof(GenValuesForParamCache2), CacheExpiration = 20)] public string Param1; protected override void EndProcessing() @@ -259,7 +272,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { public class TestValidateSetCommand3 : PSCmdlet { [Parameter] - [ValidateSet(typeof(GenValuesForParam1Cache), CacheExpiration = 2)] + [ValidateSet(typeof(GenValuesForParamCache2), CacheExpiration = 2)] public string Param1; protected override void EndProcessing() @@ -268,10 +281,44 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } + [Cmdlet(VerbsCommon.Get, "TestValidateSet4")] + public class TestValidateSetCommand4 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParam))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } + + [Cmdlet(VerbsCommon.Get, "TestValidateSet5")] + public class TestValidateSetCommand5 : PSCmdlet + { + [Parameter] + [ValidateSet(typeof(GenValuesForParamNull))] + public string Param1; + + protected override void EndProcessing() + { + WriteObject(Param1); + } + } /// Implement of test IValidateSetValuesGenerator - public class GenValuesForParam1 : IValidateSetValuesGenerator + public class GenValuesForParamNull : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() + { + var testValues = new string[] {"Test1","TestString1","Test2"}; + return null; + } + } + + public class GenValuesForParam : IValidateSetValuesGenerator { public IEnumerable GetValidValues() { @@ -283,7 +330,32 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } - public class GenValuesForParam1Cache : IValidateSetValuesGenerator + public class GenValuesForParamCache1 : IValidateSetValuesGenerator + { + public IEnumerable GetValidValues() + { + var testValues1 = new string[] {"Test11","TestString11","Test22"}; + var testValues2 = new string[] {"Test11","TestString22","Test22"}; + string[] testValues; + + var currentTime = DateTime.Now; + if (DateTime.Compare(_cacheTime, currentTime) <= 0) + { + testValues = testValues1; + } + else + { + testValues = testValues2; + } + foreach (var value in testValues) + { + yield return value; + } + } + private static DateTime _cacheTime = DateTime.Now.AddSeconds(2); + } + + public class GenValuesForParamCache2 : IValidateSetValuesGenerator { public IEnumerable GetValidValues() { @@ -311,88 +383,221 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } '@ - Add-Type -TypeDefinition $a -PassThru | % {$_.assembly} | Import-module -Force + Add-Type -TypeDefinition $a -PassThru | ForEach-Object {$_.assembly} | Import-module -Force + } - class GenValuesForParam : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + It 'Throw if IValidateSetValuesGenerator is not implemented' { + { Get-TestValidateSet0 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" + } - return [string[]]("Test1","TestString1","Test2") - } + It 'Dynamically generated set works in C# with cache expiration' { + Get-TestValidateSet1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" + Get-TestValidateSet1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" + Start-Sleep -Seconds 3 + Get-TestValidateSet1 -Param1 "TestString11" -ErrorAction SilentlyContinue | Should BeExactly "TestString11" + } - [DateTime] $cacheTime; + It 'Dynamically generated set works in C# with cache expiration and some ValidateSet instances' { + # Cache timeout = 20 sec. + # Fill the valid values cache. + Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Use the valid values cache. + Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Cache timeout = 2 sec. + # Use the valid values cache. + Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Use the valid values cache. + Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + + Start-Sleep -Seconds 3 + + # Cache timeout still = 20 sec. + # Use the valid values cache. + Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Next command use the same 'GenValuesForParamCache2' but reset cache timeout from 20 sec to 2 sec. + # Update the valid values cache. + Get-TestValidateSet3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + # Now cache is already updated. + # Use the valid values cache. + Get-TestValidateSet2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" } - # Return '$testValues2' and after 2 seconds after first use return another array '$testValues1'. - class GenValuesForParamCache : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + It 'Dynamically generated set works in C# with default (immediate) cache expire' { + Get-TestValidateSet4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } - $testValues1 = "Test1","TestString1","Test2" - $testValues2 = "Test1","TestString2","Test2" + It 'Empty dynamically generated set throws in C#' { + $exc = { + Get-TestValidateSet5 -Param1 "TestString1" -ErrorAction Stop + } | ShouldBeErrorId "ParameterArgumentValidationError,Test.Language.TestValidateSetCommand5" + $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + } - $currentTime = [DateTime]::Now - if ([DateTime]::Compare([GenValuesForParamCache]::cacheTime, $currentTime) -le 0) - { - $testValues = $testValues1; + + } + + Context 'Powershell tests' { + + BeforeAll { + class GenValuesForParam : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + + return [string[]]("Test1","TestString1","Test2") } - else - { - $testValues = $testValues2; + } + + class GenValuesForParamNull : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + + return [string[]]$null } - return [string[]]$testValues } - static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); - } + # Return '$testValues2' and after 2 seconds after first use return another array '$testValues1'. + class GenValuesForParamCache1 : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + + $testValues1 = "Test11","TestString11","Test22" + $testValues2 = "Test11","TestString22","Test22" + + $currentTime = [DateTime]::Now + if ([DateTime]::Compare([GenValuesForParamCache1]::cacheTime, $currentTime) -le 0) + { + $testValues = $testValues1; + } + else + { + $testValues = $testValues2; + } + return [string[]]$testValues + } + + static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); + } + + class GenValuesForParamCache2 : System.Management.Automation.IValidateSetValuesGenerator { + [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + + $testValues1 = "Test1","TestString1","Test2" + $testValues2 = "Test1","TestString2","Test2" + + $currentTime = [DateTime]::Now + if ([DateTime]::Compare([GenValuesForParamCache2]::cacheTime, $currentTime) -le 0) + { + $testValues = $testValues1; + } + else + { + $testValues = $testValues2; + } + return [string[]]$testValues + } + + static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); + } + + function Get-TestValidateSetPS1 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParamCache1], CacheExpiration = 2)] + $Param1 + ) - function Get-TestValidateSet4 - { - [CmdletBinding()] - Param - ( - [ValidateSet([GenValuesForParam])] $Param1 - ) + } - $Param1 - } + function Get-TestValidateSetPS2 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParamCache2], CacheExpiration = 20)] + $Param1 + ) - function Get-TestValidateSet5 - { - [CmdletBinding()] - Param - ( - [ValidateSet([GenValuesForParamCache], CacheExpiration = 2)] $Param1 - ) + } - $Param1 - } + function Get-TestValidateSetPS3 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParamCache2], CacheExpiration = 2)] + $Param1 + ) - } + $Param1 + } - It 'Throw if IValidateSetValuesGenerator is not implemented' { - { Get-TestValidateSet1 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" - } + function Get-TestValidateSetPS4 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParam])] + $Param1 + ) - It 'Dynamically generated set work in C#' { - Get-TestValidateSet2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - } + $Param1 + } - It 'Dynamically generated set work in C# with cache expiration' { - Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - Start-Sleep -Seconds 2 - Get-TestValidateSet3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - } + function Get-TestValidateSetPS5 + { + [CmdletBinding()] + Param + ( + [ValidateSet([GenValuesForParamNull])] + $Param1 + ) - It 'Dynamically generated set work in PowerShell script' { - Get-TestValidateSet4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - } + $Param1 + } + } + + It 'Dynamically generated set works in PowerShell script with cache expiration' { + Get-TestValidateSetPS1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" + Get-TestValidateSetPS1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" + Start-Sleep -Seconds 3 + Get-TestValidateSetPS1 -Param1 "TestString11" -ErrorAction SilentlyContinue | Should BeExactly "TestString11" + } + + It 'Dynamically generated set works in PowerShell script with cache expiration and some ValidateSet instances' { + # Cache timeout = 20 sec. + # Fill the valid values cache. + Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Use the valid values cache. + Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Cache timeout = 2 sec. + # Use the valid values cache. + Get-TestValidateSetPS3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Use the valid values cache. + Get-TestValidateSetPS3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + + Start-Sleep -Seconds 3 + + # Cache timeout still = 20 sec. + # Use the valid values cache. + Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" + # Next command use the same 'GenValuesForParamCache2' but reset cache timeout from 20 sec to 2 sec. + # Update the valid values cache. + Get-TestValidateSetPS3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + # Now cache is already updated. + # Use the valid values cache. + Get-TestValidateSetPS2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } + + It 'Dynamically generated set works in PowerShell script with default (immediate) cache expire' { + Get-TestValidateSetPS4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } - It 'Dynamically generated set work in PowerShell script with cache expiration' { - Get-TestValidateSet5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - Get-TestValidateSet5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - Start-Sleep -Seconds 2 - Get-TestValidateSet5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + It 'Empty dynamically generated set throws in PowerShell script' { + $exc = { + Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction Stop + } | ShouldBeErrorId "ParameterArgumentValidationError,Get-TestValidateSetPS5" + $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + } } } From 4e1f0bbe7a14af4c3564f733f25ca520a05cab08 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 1 Jun 2017 18:41:26 +0300 Subject: [PATCH 07/20] Fix 'NewValidateSetAttribute' and add test for unimplemented generator type --- .../engine/parser/Compiler.cs | 35 ++++++++++++++++--- .../resources/ParserStrings.resx | 3 ++ .../Scripting.Classes.Attributes.Tests.ps1 | 18 ++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index d3253034a66..a73b70fa619 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -739,6 +739,7 @@ static Compiler() s_builtinAttributeGenerator.Add(typeof(ParameterAttribute), NewParameterAttribute); s_builtinAttributeGenerator.Add(typeof(OutputTypeAttribute), NewOutputTypeAttribute); s_builtinAttributeGenerator.Add(typeof(AliasAttribute), NewAliasAttribute); + s_builtinAttributeGenerator.Add(typeof(ValidateSetAttribute), NewValidateSetAttribute); s_builtinAttributeGenerator.Add(typeof(DebuggerHiddenAttribute), NewDebuggerHiddenAttribute); s_builtinAttributeGenerator.Add(typeof(ValidateNotNullAttribute), NewValidateNotNullAttribute); s_builtinAttributeGenerator.Add(typeof(ValidateNotNullOrEmptyAttribute), NewValidateNotNullOrEmptyAttribute); @@ -1297,15 +1298,35 @@ private static Attribute NewAliasAttribute(AttributeAst ast) private static Attribute NewValidateSetAttribute(AttributeAst ast) { + ValidateSetAttribute result; var cvv = new ConstantValueVisitor { AttributeArgument = true }; - var args = new string[ast.PositionalArguments.Count]; - for (int i = 0; i < ast.PositionalArguments.Count; i++) + + // 'ValidateSet([CustomGeneratorType], IgnoreCase=$false)' is supported in scripts. + if (ast.PositionalArguments.Count == 1 && ast.PositionalArguments[0] is TypeExpressionAst generatorTypeAst) { - args[i] = _attrArgToStringConverter.Target(_attrArgToStringConverter, - ast.PositionalArguments[i].Accept(cvv)); + if (TypeResolver.TryResolveType(generatorTypeAst.TypeName.FullName, out Type generatorType)) + { + result = new ValidateSetAttribute(generatorType); + } + else + { + throw InterpreterError.NewInterpreterException(ast, typeof(RuntimeException), ast.Extent, + "CustomValidValuesGeneratorTypeNotFound", ParserStrings.CustomValidValuesGeneratorTypeNotFound, generatorTypeAst.TypeName.FullName, typeof(System.Management.Automation.IValidateSetValuesGenerator).FullName); + } + } + else + { + // 'ValidateSet("value1","value2", IgnoreCase=$false)' is supported in scripts. + var args = new string[ast.PositionalArguments.Count]; + for (int i = 0; i < ast.PositionalArguments.Count; i++) + { + args[i] = _attrArgToStringConverter.Target(_attrArgToStringConverter, + ast.PositionalArguments[i].Accept(cvv)); + } + + result = new ValidateSetAttribute(args); } - var result = new ValidateSetAttribute(args); foreach (var namedArg in ast.NamedArguments) { var argValue = namedArg.Argument.Accept(cvv); @@ -1314,6 +1335,10 @@ private static Attribute NewValidateSetAttribute(AttributeAst ast) { result.IgnoreCase = s_attrArgToBoolConverter.Target(s_attrArgToBoolConverter, argValue); } + else if (argumentName.Equals("CacheExpiration", StringComparison.OrdinalIgnoreCase)) + { + result.CacheExpiration = s_attrArgToIntConverter.Target(s_attrArgToIntConverter, argValue); + } else if (argumentName.Equals("ErrorMessage", StringComparison.OrdinalIgnoreCase)) { result.ErrorMessage = argValue.ToString(); diff --git a/src/System.Management.Automation/resources/ParserStrings.resx b/src/System.Management.Automation/resources/ParserStrings.resx index 4390b762e87..db0d3a73511 100644 --- a/src/System.Management.Automation/resources/ParserStrings.resx +++ b/src/System.Management.Automation/resources/ParserStrings.resx @@ -759,6 +759,9 @@ Possible matches are Cannot find the type for custom attribute '{0}'. Make sure that the assembly that contains this type is loaded. + + Cannot find the valid values generator type with name '{0}'. Make sure that the type is defined and implement '{1}' interface. + Cannot find an appropriate constructor to instantiate the custom attribute object for type '{0}'. diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index a3f94300ea0..ae44ad7fb02 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -555,6 +555,18 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { $Param1 } + + function Get-TestValidateSetPS6 + { + [CmdletBinding()] + Param + ( + [ValidateSet([UnImplementedGeneratorOfValues])] + $Param1 + ) + + $Param1 + } } It 'Dynamically generated set works in PowerShell script with cache expiration' { @@ -599,5 +611,11 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } | ShouldBeErrorId "ParameterArgumentValidationError,Get-TestValidateSetPS5" $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" } + + It 'Unimplemented valid values generator type throws in PowerShell script' { + $exc = { + Get-TestValidateSetPS6 -Param1 "AnyTestString" -ErrorAction Stop + } | ShouldBeErrorId "CustomValidValuesGeneratorTypeNotFound" + } } } From 5dccbad1e0cca21ccc293c1043d60bfb04de56a8 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 9 Jun 2017 18:15:27 +0300 Subject: [PATCH 08/20] Remove a valid values cache from ValidateSet attribute 1. Remove a valid values cache from ValidateSet attribute. 2. Enhance a valid value generator cache. 3. Add an abstract class for valid value generator with valid values cache support. --- .../engine/Attributes.cs | 177 +++++++++++++++--- .../engine/parser/Compiler.cs | 4 - 2 files changed, 148 insertions(+), 33 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index d84671e17a5..58d6fcaa065 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1355,6 +1355,53 @@ public ValidateCountAttribute(int minLength, int maxLength) } } + /// + /// Base class for all valid values generators with cache support. + /// + public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerator + { + // Cached valid values. + private string[] _validValues; + + private DateTime cacheCreateTime = DateTime.MinValue; + + /// + /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. + /// By default 'ValidValuesCacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). + /// + public int ValidValuesCacheExpiration { get; set; } = 0; + + /// + /// Abstract method to generate a valid values. + /// + public abstract string[] GenerateValidValues(); + + /// + /// Get a valid values. + /// + public IEnumerable GetValidValues() + { + var currentTime = DateTime.Now; + if (_validValues != null && DateTime.Compare(cacheCreateTime.AddSeconds(ValidValuesCacheExpiration), currentTime) > 0) + { + return _validValues; + } + + _validValues = GenerateValidValues(); + cacheCreateTime = currentTime; + + if (_validValues == null) + { + throw new ValidationMetadataException( + "ValidateSetGeneratedValidValuesListIsEmpty", + null, + Metadata.ValidateSetGeneratedValidValuesListIsEmpty); + } + + return _validValues; + } + } + /// /// Validates that each parameter argument is present in a specified set /// @@ -1365,29 +1412,98 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute // or dynamic valid values list generated by instance of 'validValuesGeneratorType'. private string[] _validValues; - // The valid values cache works across 'ValidateSetAttribute' instances. - // By default 'CacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). - private static ConcurrentDictionary validValuesCache = new ConcurrentDictionary(); - private class ValidValuesCacheEntry + // The valid values generator cache works across 'ValidateSetAttribute' instances. + private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); + + // The default is from CoreFX for 'ConcurrentDictionary' type. + // https://github.com/dotnet/corefx/blob/master/src/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs#L73 + // Value = 0 - disable the cache and recreate a valid values generator in each call 'ValidValues'. + internal static int s_validValuesGeneratorCacheSize = 31; + + // We should adjust the default value based on a user feedback. + // Since we have a public interface to configure the value, we can set a large default value. + internal static int s_validValuesGeneratorCacheExpiration = 900; + + /// + /// Allow get/set the valid values generator cache expiration at runtime. + /// + public static int ValidValuesGeneratorCacheExpiration + { + get + { + return s_validValuesGeneratorCacheExpiration; + } + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException("ValidValuesGeneratorCacheSize"); + } + + s_validValuesGeneratorCacheExpiration = value; + + CleanExpiredValidValuesGeneratorCache(); + } + } + + /// + /// Allow get/set the valid values generator cache size at runtime. + /// + public static int ValidValuesGeneratorCacheSize + { + get + { + return s_validValuesGeneratorCacheSize; + } + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException("ValidValuesGeneratorCacheSize"); + } + + s_validValuesGeneratorCacheSize = value; + + AdjustValidValuesGeneratorCache(); + } + } + private static void AdjustValidValuesGeneratorCache() + { + CleanExpiredValidValuesGeneratorCache(); + // If after removing all expired entries the cache still remains overflowed + // we should increase a size of the cache but this is a user privilege. + // So here we can only remove random items. + while (s_ValidValuesGeneratorCache.Count > s_validValuesGeneratorCacheSize) + { + s_ValidValuesGeneratorCache.First(); + } + } + + private static void CleanExpiredValidValuesGeneratorCache() + { + var currentTime = DateTime.Now; + foreach (var item in s_ValidValuesGeneratorCache) + { + if (DateTime.Compare(item.Value.cacheEntryAccessTime.AddSeconds(ValidValuesGeneratorCacheExpiration), currentTime) < 0) + { + s_ValidValuesGeneratorCache.TryRemove(item.Key, out var ignore); + } + } + } + private class ValidValuesGeneratorCacheEntry { public IValidateSetValuesGenerator validValuesGenerator = null; - public string[] validValues; - public DateTime cacheEntryCreationTime = DateTime.MinValue; - public ValidValuesCacheEntry(IValidateSetValuesGenerator validValuesGenerator, string[] validValues, DateTime cacheEntryCreationTime) + public DateTime cacheEntryAccessTime = DateTime.MinValue; + + public ValidValuesGeneratorCacheEntry(IValidateSetValuesGenerator validValuesGenerator, DateTime cacheEntryAccessTime) { this.validValuesGenerator = validValuesGenerator; - this.validValues = validValues; - this.cacheEntryCreationTime = cacheEntryCreationTime; + this.cacheEntryAccessTime = cacheEntryAccessTime; } } private Type validValuesGeneratorType = null; - /// - /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. - /// - public int CacheExpiration { get; set; } = 0; - /// /// Gets or sets the custom error message that is displayed to the user /// @@ -1415,34 +1531,37 @@ public IList ValidValues { if (validValuesGeneratorType != null) { - ValidValuesCacheEntry validValuesCacheEntry; - if (validValuesCache.TryGetValue(validValuesGeneratorType, out validValuesCacheEntry)) + ValidValuesGeneratorCacheEntry ValidValuesGeneratorCacheEntry; + if (s_ValidValuesGeneratorCache.TryGetValue(validValuesGeneratorType, out ValidValuesGeneratorCacheEntry)) { + // The valid values generator is in the cache. + // Update last access time. We use this for cache cleanup. var currentTime = DateTime.Now; - if (DateTime.Compare(validValuesCacheEntry.cacheEntryCreationTime.AddSeconds(CacheExpiration), currentTime) < 0) - { - // Update expired Entry. - validValuesCacheEntry.validValues = validValuesCacheEntry.validValuesGenerator.GetValidValues()?.ToArray(); - validValuesCacheEntry.cacheEntryCreationTime = currentTime; - validValuesCache[validValuesGeneratorType] = validValuesCacheEntry; - } + ValidValuesGeneratorCacheEntry.cacheEntryAccessTime = currentTime; + s_ValidValuesGeneratorCache[validValuesGeneratorType] = ValidValuesGeneratorCacheEntry; } else { + // Add a valid values generator to the cache. + // We don't cache valid values. + // We expect that valid values can be cached in the valid values generator. var validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); - var validValues = validValuesGenerator.GetValidValues()?.ToArray(); - var cacheEntryCreationTime = DateTime.Now; - validValuesCacheEntry = new ValidValuesCacheEntry(validValuesGenerator, validValues, cacheEntryCreationTime); - validValuesCache.TryAdd(validValuesGeneratorType, validValuesCacheEntry); + var cacheEntryAccessTime = DateTime.Now; + ValidValuesGeneratorCacheEntry = new ValidValuesGeneratorCacheEntry(validValuesGenerator, cacheEntryAccessTime); + s_ValidValuesGeneratorCache.TryAdd(validValuesGeneratorType, ValidValuesGeneratorCacheEntry); } - if (validValuesCacheEntry.validValues == null) + + _validValues = ValidValuesGeneratorCacheEntry.validValuesGenerator.GetValidValues()?.ToArray(); + + AdjustValidValuesGeneratorCache(); + + if (_validValues == null) { throw new ValidationMetadataException( "ValidateSetGeneratedValidValuesListIsEmpty", null, Metadata.ValidateSetGeneratedValidValuesListIsEmpty); } - _validValues = validValuesCacheEntry.validValues; } return _validValues; } diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index a73b70fa619..bb0b4a5ccb0 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -1335,10 +1335,6 @@ private static Attribute NewValidateSetAttribute(AttributeAst ast) { result.IgnoreCase = s_attrArgToBoolConverter.Target(s_attrArgToBoolConverter, argValue); } - else if (argumentName.Equals("CacheExpiration", StringComparison.OrdinalIgnoreCase)) - { - result.CacheExpiration = s_attrArgToIntConverter.Target(s_attrArgToIntConverter, argValue); - } else if (argumentName.Equals("ErrorMessage", StringComparison.OrdinalIgnoreCase)) { result.ErrorMessage = argValue.ToString(); From a1f1aacc800344a80f8c5ff56ed8104635c3dc46 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Tue, 13 Jun 2017 17:20:39 +0300 Subject: [PATCH 09/20] Refactor CachedValidValuesGeneratorBase class Remove CacheExpiration tests Add CachedValidValuesGeneratorBase class tests --- .../engine/Attributes.cs | 19 +- .../Scripting.Classes.Attributes.Tests.ps1 | 288 +++++------------- 2 files changed, 93 insertions(+), 214 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 58d6fcaa065..be4298cf2cc 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -12,6 +12,7 @@ using System.Management.Automation.Language; using System.Runtime.CompilerServices; using System.Linq; +using System.Threading.Tasks; namespace System.Management.Automation.Internal { @@ -1363,8 +1364,6 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat // Cached valid values. private string[] _validValues; - private DateTime cacheCreateTime = DateTime.MinValue; - /// /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. /// By default 'ValidValuesCacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). @@ -1381,16 +1380,14 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat /// public IEnumerable GetValidValues() { - var currentTime = DateTime.Now; - if (_validValues != null && DateTime.Compare(cacheCreateTime.AddSeconds(ValidValuesCacheExpiration), currentTime) > 0) + if (_validValues != null) { return _validValues; } - _validValues = GenerateValidValues(); - cacheCreateTime = currentTime; + var _validValuesNoCache = GenerateValidValues(); - if (_validValues == null) + if (_validValuesNoCache == null) { throw new ValidationMetadataException( "ValidateSetGeneratedValidValuesListIsEmpty", @@ -1398,7 +1395,13 @@ public IEnumerable GetValidValues() Metadata.ValidateSetGeneratedValidValuesListIsEmpty); } - return _validValues; + if (ValidValuesCacheExpiration > 0) + { + Task.Delay(ValidValuesCacheExpiration).ContinueWith((task) => _validValues = null); + _validValues = _validValuesNoCache; + } + + return _validValuesNoCache; } } diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index ae44ad7fb02..1d695469dca 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -219,6 +219,26 @@ Describe 'Type resolution with attributes' -Tag "CI" { Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { + Context 'Common tests' { + It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheExpiration' { + [ValidateSet]::ValidValuesGeneratorCacheExpiration | Should Be 900 + } + + It 'Can set value [ValidateSet]::ValidValuesGeneratorCacheExpiration' { + [ValidateSet]::ValidValuesGeneratorCacheExpiration = 60 + [ValidateSet]::ValidValuesGeneratorCacheExpiration | Should Be 60 + } + + It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheSize' { + [ValidateSet]::ValidValuesGeneratorCacheSize | Should Be 31 + } + + It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheSize' { + [ValidateSet]::ValidValuesGeneratorCacheSize = 15 + [ValidateSet]::ValidValuesGeneratorCacheSize | Should Be 15 + } +} + Context 'C# tests' { BeforeAll { @@ -242,45 +262,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } - [Cmdlet(VerbsCommon.Get, "TestValidateSet1")] - public class TestValidateSetCommand1 : PSCmdlet - { - [Parameter] - [ValidateSet(typeof(GenValuesForParamCache1), CacheExpiration = 2)] - public string Param1; - - protected override void EndProcessing() - { - WriteObject(Param1); - } - } - - [Cmdlet(VerbsCommon.Get, "TestValidateSet2")] - public class TestValidateSetCommand2 : PSCmdlet - { - [Parameter] - [ValidateSet(typeof(GenValuesForParamCache2), CacheExpiration = 20)] - public string Param1; - - protected override void EndProcessing() - { - WriteObject(Param1); - } - } - - [Cmdlet(VerbsCommon.Get, "TestValidateSet3")] - public class TestValidateSetCommand3 : PSCmdlet - { - [Parameter] - [ValidateSet(typeof(GenValuesForParamCache2), CacheExpiration = 2)] - public string Param1; - - protected override void EndProcessing() - { - WriteObject(Param1); - } - } - [Cmdlet(VerbsCommon.Get, "TestValidateSet4")] public class TestValidateSetCommand4 : PSCmdlet { @@ -329,57 +310,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } } - - public class GenValuesForParamCache1 : IValidateSetValuesGenerator - { - public IEnumerable GetValidValues() - { - var testValues1 = new string[] {"Test11","TestString11","Test22"}; - var testValues2 = new string[] {"Test11","TestString22","Test22"}; - string[] testValues; - - var currentTime = DateTime.Now; - if (DateTime.Compare(_cacheTime, currentTime) <= 0) - { - testValues = testValues1; - } - else - { - testValues = testValues2; - } - foreach (var value in testValues) - { - yield return value; - } - } - private static DateTime _cacheTime = DateTime.Now.AddSeconds(2); - } - - public class GenValuesForParamCache2 : IValidateSetValuesGenerator - { - public IEnumerable GetValidValues() - { - var testValues1 = new string[] {"Test1","TestString1","Test2"}; - var testValues2 = new string[] {"Test1","TestString2","Test2"}; - string[] testValues; - - var currentTime = DateTime.Now; - if (DateTime.Compare(_cacheTime, currentTime) <= 0) - { - testValues = testValues1; - //_cacheTime = currentTime.AddSeconds(2); - } - else - { - testValues = testValues2; - } - foreach (var value in testValues) - { - yield return value; - } - } - private static DateTime _cacheTime = DateTime.Now.AddSeconds(2); - } } '@ @@ -390,38 +320,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { { Get-TestValidateSet0 -Param1 "TestString" -ErrorAction Stop } | ShouldBeErrorId "Argument" } - It 'Dynamically generated set works in C# with cache expiration' { - Get-TestValidateSet1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" - Get-TestValidateSet1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" - Start-Sleep -Seconds 3 - Get-TestValidateSet1 -Param1 "TestString11" -ErrorAction SilentlyContinue | Should BeExactly "TestString11" - } - - It 'Dynamically generated set works in C# with cache expiration and some ValidateSet instances' { - # Cache timeout = 20 sec. - # Fill the valid values cache. - Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Use the valid values cache. - Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Cache timeout = 2 sec. - # Use the valid values cache. - Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Use the valid values cache. - Get-TestValidateSet3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - - Start-Sleep -Seconds 3 - - # Cache timeout still = 20 sec. - # Use the valid values cache. - Get-TestValidateSet2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Next command use the same 'GenValuesForParamCache2' but reset cache timeout from 20 sec to 2 sec. - # Update the valid values cache. - Get-TestValidateSet3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - # Now cache is already updated. - # Use the valid values cache. - Get-TestValidateSet2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - } - It 'Dynamically generated set works in C# with default (immediate) cache expire' { Get-TestValidateSet4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" } @@ -432,8 +330,6 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } | ShouldBeErrorId "ParameterArgumentValidationError,Test.Language.TestValidateSetCommand5" $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" } - - } Context 'Powershell tests' { @@ -475,93 +371,110 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); } - class GenValuesForParamCache2 : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { - - $testValues1 = "Test1","TestString1","Test2" - $testValues2 = "Test1","TestString2","Test2" - - $currentTime = [DateTime]::Now - if ([DateTime]::Compare([GenValuesForParamCache2]::cacheTime, $currentTime) -le 0) - { - $testValues = $testValues1; - } - else - { - $testValues = $testValues2; - } - return [string[]]$testValues - } - - static [DateTime] $cacheTime = [DateTime]::Now.AddSeconds(2); - } - - function Get-TestValidateSetPS1 + function Get-TestValidateSetPS4 { [CmdletBinding()] Param ( - [ValidateSet([GenValuesForParamCache1], CacheExpiration = 2)] + [ValidateSet([GenValuesForParam])] $Param1 ) $Param1 } - function Get-TestValidateSetPS2 + function Get-TestValidateSetPS5 { [CmdletBinding()] Param ( - [ValidateSet([GenValuesForParamCache2], CacheExpiration = 20)] + [ValidateSet([GenValuesForParamNull])] $Param1 ) $Param1 } - function Get-TestValidateSetPS3 + function Get-TestValidateSetPS6 { [CmdletBinding()] Param ( - [ValidateSet([GenValuesForParamCache2], CacheExpiration = 2)] + [ValidateSet([UnImplementedGeneratorOfValues])] $Param1 ) $Param1 } + } - function Get-TestValidateSetPS4 + It 'Dynamically generated set works in PowerShell script with default (immediate) cache expire' { + Get-TestValidateSetPS4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + } + + It 'Empty dynamically generated set throws in PowerShell script' { + $exc = { + Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction Stop + } | ShouldBeErrorId "ParameterArgumentValidationError,Get-TestValidateSetPS5" + $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + } + + It 'Unimplemented valid values generator type throws in PowerShell script' { { - [CmdletBinding()] - Param - ( - [ValidateSet([GenValuesForParam])] - $Param1 - ) + Get-TestValidateSetPS6 -Param1 "AnyTestString" -ErrorAction Stop + } | ShouldBeErrorId "CustomValidValuesGeneratorTypeNotFound" + } + } - $Param1 + Context 'CachedValidValuesGeneratorBase class tests' { + + BeforeAll { + class GenValuesForParam : System.Management.Automation.CachedValidValuesGeneratorBase { + [String[]] GenerateValidValues() { + + return [string[]]("Test1","TestString1","Test2") + } } - function Get-TestValidateSetPS5 + class GenValuesWithExpiration : System.Management.Automation.CachedValidValuesGeneratorBase { + GenValuesWithExpiration() : base() { + $this.ValidValuesCacheExpiration = 2 + } + + Static [bool] $temp = $true; + + [String[]] GenerateValidValues() { + + if ([GenValuesWithExpiration]::temp) { + [GenValuesWithExpiration]::temp = $false + return [string[]]("Test1","TestString1","Test2") + } else { + [GenValuesWithExpiration]::temp = $true + return [string[]]("Test1","TestString2","Test2") + } + + } + } + + + function Get-TestValidateSetPS4 { [CmdletBinding()] Param ( - [ValidateSet([GenValuesForParamNull])] + [ValidateSet([GenValuesForParam])] $Param1 ) $Param1 } - function Get-TestValidateSetPS6 + function Get-TestValidateSetPS5 { [CmdletBinding()] Param ( - [ValidateSet([UnImplementedGeneratorOfValues])] + [ValidateSet([GenValuesWithExpiration])] $Param1 ) @@ -569,53 +482,16 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } } - It 'Dynamically generated set works in PowerShell script with cache expiration' { - Get-TestValidateSetPS1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" - Get-TestValidateSetPS1 -Param1 "TestString22" -ErrorAction SilentlyContinue | Should BeExactly "TestString22" - Start-Sleep -Seconds 3 - Get-TestValidateSetPS1 -Param1 "TestString11" -ErrorAction SilentlyContinue | Should BeExactly "TestString11" - } - - It 'Dynamically generated set works in PowerShell script with cache expiration and some ValidateSet instances' { - # Cache timeout = 20 sec. - # Fill the valid values cache. - Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Use the valid values cache. - Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Cache timeout = 2 sec. - # Use the valid values cache. - Get-TestValidateSetPS3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Use the valid values cache. - Get-TestValidateSetPS3 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - - Start-Sleep -Seconds 3 - - # Cache timeout still = 20 sec. - # Use the valid values cache. - Get-TestValidateSetPS2 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" - # Next command use the same 'GenValuesForParamCache2' but reset cache timeout from 20 sec to 2 sec. - # Update the valid values cache. - Get-TestValidateSetPS3 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - # Now cache is already updated. - # Use the valid values cache. - Get-TestValidateSetPS2 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - } - - It 'Dynamically generated set works in PowerShell script with default (immediate) cache expire' { + It 'Can implement CachedValidValuesGeneratorBase in PowerShell' { Get-TestValidateSetPS4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" } - It 'Empty dynamically generated set throws in PowerShell script' { - $exc = { - Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction Stop - } | ShouldBeErrorId "ParameterArgumentValidationError,Get-TestValidateSetPS5" - $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + It 'Can implement CachedValidValuesGeneratorBase in PowerShell' { + Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" + Start-Sleep 2 + Get-TestValidateSetPS5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" } - It 'Unimplemented valid values generator type throws in PowerShell script' { - $exc = { - Get-TestValidateSetPS6 -Param1 "AnyTestString" -ErrorAction Stop - } | ShouldBeErrorId "CustomValidValuesGeneratorTypeNotFound" - } } } From 013ef8570e84ddf59720336a90839240216f506c Mon Sep 17 00:00:00 2001 From: iSazonov Date: Wed, 14 Jun 2017 08:27:22 +0300 Subject: [PATCH 10/20] Fix race condition, fix local name --- .../engine/Attributes.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index be4298cf2cc..76ba947e1af 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1380,14 +1380,17 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat /// public IEnumerable GetValidValues() { - if (_validValues != null) + // Because we have a background task to clear the cache by '_validValues = null' + // we use the local variable to exclude a race condition. + var validValuesLocal = _validValues; + if (validValuesLocal != null) { - return _validValues; + return validValuesLocal; } - var _validValuesNoCache = GenerateValidValues(); + var validValuesNoCache = GenerateValidValues(); - if (_validValuesNoCache == null) + if (validValuesNoCache == null) { throw new ValidationMetadataException( "ValidateSetGeneratedValidValuesListIsEmpty", @@ -1398,10 +1401,10 @@ public IEnumerable GetValidValues() if (ValidValuesCacheExpiration > 0) { Task.Delay(ValidValuesCacheExpiration).ContinueWith((task) => _validValues = null); - _validValues = _validValuesNoCache; + _validValues = validValuesNoCache; } - return _validValuesNoCache; + return validValuesNoCache; } } From f2cfc639fc6fab8dae72c7c672c35fe061207953 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 15 Jun 2017 10:14:48 +0300 Subject: [PATCH 11/20] Fix validValuesGenerator Change IValidateSetValuesGenerator (return type string{}) Now ValidValuesCacheExpiration in seconds Remove validValuesGeneratorCache cleanup Move generator creation to constructor Fix tests --- .../engine/Attributes.cs | 155 ++++-------------- .../Scripting.Classes.Attributes.Tests.ps1 | 39 +---- 2 files changed, 43 insertions(+), 151 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 76ba947e1af..f688a6e319e 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1368,8 +1368,18 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. /// By default 'ValidValuesCacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). /// - public int ValidValuesCacheExpiration { get; set; } = 0; + public int ValidValuesCacheExpiration { + get + { + return _ValidValuesCacheExpiration / 1000; + } + set + { + _ValidValuesCacheExpiration = value * 1000; + } + } + private int _ValidValuesCacheExpiration; /// /// Abstract method to generate a valid values. /// @@ -1378,7 +1388,7 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat /// /// Get a valid values. /// - public IEnumerable GetValidValues() + public string[] GetValidValues() { // Because we have a background task to clear the cache by '_validValues = null' // we use the local variable to exclude a race condition. @@ -1419,95 +1429,8 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute private string[] _validValues; // The valid values generator cache works across 'ValidateSetAttribute' instances. - private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); - - // The default is from CoreFX for 'ConcurrentDictionary' type. - // https://github.com/dotnet/corefx/blob/master/src/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs#L73 - // Value = 0 - disable the cache and recreate a valid values generator in each call 'ValidValues'. - internal static int s_validValuesGeneratorCacheSize = 31; - - // We should adjust the default value based on a user feedback. - // Since we have a public interface to configure the value, we can set a large default value. - internal static int s_validValuesGeneratorCacheExpiration = 900; - - /// - /// Allow get/set the valid values generator cache expiration at runtime. - /// - public static int ValidValuesGeneratorCacheExpiration - { - get - { - return s_validValuesGeneratorCacheExpiration; - } - set - { - if (value < 0) - { - throw new ArgumentOutOfRangeException("ValidValuesGeneratorCacheSize"); - } - - s_validValuesGeneratorCacheExpiration = value; - - CleanExpiredValidValuesGeneratorCache(); - } - } - - /// - /// Allow get/set the valid values generator cache size at runtime. - /// - public static int ValidValuesGeneratorCacheSize - { - get - { - return s_validValuesGeneratorCacheSize; - } - set - { - if (value < 0) - { - throw new ArgumentOutOfRangeException("ValidValuesGeneratorCacheSize"); - } + private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); - s_validValuesGeneratorCacheSize = value; - - AdjustValidValuesGeneratorCache(); - } - } - private static void AdjustValidValuesGeneratorCache() - { - CleanExpiredValidValuesGeneratorCache(); - // If after removing all expired entries the cache still remains overflowed - // we should increase a size of the cache but this is a user privilege. - // So here we can only remove random items. - while (s_ValidValuesGeneratorCache.Count > s_validValuesGeneratorCacheSize) - { - s_ValidValuesGeneratorCache.First(); - } - } - - private static void CleanExpiredValidValuesGeneratorCache() - { - var currentTime = DateTime.Now; - foreach (var item in s_ValidValuesGeneratorCache) - { - if (DateTime.Compare(item.Value.cacheEntryAccessTime.AddSeconds(ValidValuesGeneratorCacheExpiration), currentTime) < 0) - { - s_ValidValuesGeneratorCache.TryRemove(item.Key, out var ignore); - } - } - } - private class ValidValuesGeneratorCacheEntry - { - public IValidateSetValuesGenerator validValuesGenerator = null; - - public DateTime cacheEntryAccessTime = DateTime.MinValue; - - public ValidValuesGeneratorCacheEntry(IValidateSetValuesGenerator validValuesGenerator, DateTime cacheEntryAccessTime) - { - this.validValuesGenerator = validValuesGenerator; - this.cacheEntryAccessTime = cacheEntryAccessTime; - } - } private Type validValuesGeneratorType = null; /// @@ -1535,41 +1458,25 @@ public IList ValidValues { get { - if (validValuesGeneratorType != null) + if (validValuesGeneratorType == null) { - ValidValuesGeneratorCacheEntry ValidValuesGeneratorCacheEntry; - if (s_ValidValuesGeneratorCache.TryGetValue(validValuesGeneratorType, out ValidValuesGeneratorCacheEntry)) - { - // The valid values generator is in the cache. - // Update last access time. We use this for cache cleanup. - var currentTime = DateTime.Now; - ValidValuesGeneratorCacheEntry.cacheEntryAccessTime = currentTime; - s_ValidValuesGeneratorCache[validValuesGeneratorType] = ValidValuesGeneratorCacheEntry; - } - else - { - // Add a valid values generator to the cache. - // We don't cache valid values. - // We expect that valid values can be cached in the valid values generator. - var validValuesGenerator = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); - var cacheEntryAccessTime = DateTime.Now; - ValidValuesGeneratorCacheEntry = new ValidValuesGeneratorCacheEntry(validValuesGenerator, cacheEntryAccessTime); - s_ValidValuesGeneratorCache.TryAdd(validValuesGeneratorType, ValidValuesGeneratorCacheEntry); - } + return _validValues; + } - _validValues = ValidValuesGeneratorCacheEntry.validValuesGenerator.GetValidValues()?.ToArray(); + IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry; + s_ValidValuesGeneratorCache.TryGetValue(validValuesGeneratorType, out ValidValuesGeneratorCacheEntry); - AdjustValidValuesGeneratorCache(); + var validValuesLocal = ValidValuesGeneratorCacheEntry?.GetValidValues(); - if (_validValues == null) - { - throw new ValidationMetadataException( - "ValidateSetGeneratedValidValuesListIsEmpty", - null, - Metadata.ValidateSetGeneratedValidValuesListIsEmpty); - } + if (validValuesLocal == null) + { + throw new ValidationMetadataException( + "ValidateSetGeneratedValidValuesListIsEmpty", + null, + Metadata.ValidateSetGeneratedValidValuesListIsEmpty); } - return _validValues; + + return validValuesLocal; } } @@ -1650,6 +1557,12 @@ public ValidateSetAttribute(Type valuesGeneratorType) } validValuesGeneratorType = valuesGeneratorType; + + // Add a valid values generator to the cache. + // We don't cache valid values. + // We expect that valid values can be cached in the valid values generator. + var ValidValuesGeneratorCacheEntry = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); + s_ValidValuesGeneratorCache.TryAdd(validValuesGeneratorType, ValidValuesGeneratorCacheEntry); } } @@ -1661,7 +1574,7 @@ public interface IValidateSetValuesGenerator /// /// Get a valid values. /// - IEnumerable GetValidValues(); + string[] GetValidValues(); } #region Allow diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index 1d695469dca..248ed1dd867 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -219,26 +219,6 @@ Describe 'Type resolution with attributes' -Tag "CI" { Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { - Context 'Common tests' { - It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheExpiration' { - [ValidateSet]::ValidValuesGeneratorCacheExpiration | Should Be 900 - } - - It 'Can set value [ValidateSet]::ValidValuesGeneratorCacheExpiration' { - [ValidateSet]::ValidValuesGeneratorCacheExpiration = 60 - [ValidateSet]::ValidValuesGeneratorCacheExpiration | Should Be 60 - } - - It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheSize' { - [ValidateSet]::ValidValuesGeneratorCacheSize | Should Be 31 - } - - It 'Check default value [ValidateSet]::ValidValuesGeneratorCacheSize' { - [ValidateSet]::ValidValuesGeneratorCacheSize = 15 - [ValidateSet]::ValidValuesGeneratorCacheSize | Should Be 15 - } -} - Context 'C# tests' { BeforeAll { @@ -292,7 +272,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { /// Implement of test IValidateSetValuesGenerator public class GenValuesForParamNull : IValidateSetValuesGenerator { - public IEnumerable GetValidValues() + public string[] GetValidValues() { var testValues = new string[] {"Test1","TestString1","Test2"}; return null; @@ -301,19 +281,18 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { public class GenValuesForParam : IValidateSetValuesGenerator { - public IEnumerable GetValidValues() + public string[] GetValidValues() { var testValues = new string[] {"Test1","TestString1","Test2"}; - foreach (var value in testValues) - { - yield return value; - } + return testValues; } } } '@ - Add-Type -TypeDefinition $a -PassThru | ForEach-Object {$_.assembly} | Import-module -Force + $testAssemply = "$TestDrive\tst-$(New-Guid).dll" + Add-Type -TypeDefinition $a -OutputAssembly $testAssemply + Import-Module $testAssemply } It 'Throw if IValidateSetValuesGenerator is not implemented' { @@ -336,14 +315,14 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { BeforeAll { class GenValuesForParam : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + [String[]] GetValidValues() { return [string[]]("Test1","TestString1","Test2") } } class GenValuesForParamNull : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + [String[]] GetValidValues() { return [string[]]$null } @@ -351,7 +330,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { # Return '$testValues2' and after 2 seconds after first use return another array '$testValues1'. class GenValuesForParamCache1 : System.Management.Automation.IValidateSetValuesGenerator { - [System.Collections.Generic.IEnumerable[String]] GetValidValues() { + [String[]] GetValidValues() { $testValues1 = "Test11","TestString11","Test22" $testValues2 = "Test11","TestString22","Test22" From b5466d88743d797c7da5439fc940fecc94a749c2 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Wed, 28 Jun 2017 18:40:55 +0300 Subject: [PATCH 12/20] Minor corrections 1. Now 'ValidValuesCacheExpiration' is 'protected' 2. Correct resource string 3. Correct tests --- .../engine/Attributes.cs | 33 ++++++++++--------- .../resources/Metadata.resx | 2 +- .../Scripting.Classes.Attributes.Tests.ps1 | 6 ++-- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index f688a6e319e..bcb9c697f9c 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1118,7 +1118,7 @@ public sealed class ValidatePatternAttribute : ValidateEnumeratedArgumentsAttrib /// /// Gets or sets the custom error message pattern that is displayed to the user. - /// + /// /// The text representation of the object being validated and the validating regex is passed as /// the first and second formatting parameters to the ErrorMessage formatting pattern. /// @@ -1180,10 +1180,10 @@ public sealed class ValidateScriptAttribute : ValidateEnumeratedArgumentsAttribu { /// /// Gets or sets the custom error message that is displayed to the user. - /// + /// /// The item being validated and the validating scriptblock is passed as the first and second /// formatting argument. - /// + /// /// /// [ValidateScript("$_ % 2", ErrorMessage = "The item '{0}' did not pass validation of script '{1}'")] /// @@ -1357,7 +1357,7 @@ public ValidateCountAttribute(int minLength, int maxLength) } /// - /// Base class for all valid values generators with cache support. + /// Optional base class for implementations that want a default implementation to cache valid values. /// public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerator { @@ -1368,18 +1368,19 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. /// By default 'ValidValuesCacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). /// - public int ValidValuesCacheExpiration { + protected int ValidValuesCacheExpiration { get { - return _ValidValuesCacheExpiration / 1000; + return _validValuesCacheExpiration / 1000; } set { - _ValidValuesCacheExpiration = value * 1000; + _validValuesCacheExpiration = value * 1000; } } - private int _ValidValuesCacheExpiration; + private int _validValuesCacheExpiration; + /// /// Abstract method to generate a valid values. /// @@ -1403,14 +1404,14 @@ public string[] GetValidValues() if (validValuesNoCache == null) { throw new ValidationMetadataException( - "ValidateSetGeneratedValidValuesListIsEmpty", + "ValidateSetGeneratedValidValuesListIsNull", null, - Metadata.ValidateSetGeneratedValidValuesListIsEmpty); + Metadata.ValidateSetGeneratedValidValuesListIsNull); } - if (ValidValuesCacheExpiration > 0) + if (_validValuesCacheExpiration > 0) { - Task.Delay(ValidValuesCacheExpiration).ContinueWith((task) => _validValues = null); + Task.Delay(_validValuesCacheExpiration).ContinueWith((task) => _validValues = null); _validValues = validValuesNoCache; } @@ -1435,10 +1436,10 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute /// /// Gets or sets the custom error message that is displayed to the user - /// + /// /// The item being validated and a text representation of the validation set /// is passed as the first and second formatting argument to the ErrorMessage formatting pattern. - /// + /// /// /// [ValidateSet("A","B","C", ErrorMessage="The item '{0}' is not part of the set '{1}'.") /// @@ -1471,9 +1472,9 @@ public IList ValidValues if (validValuesLocal == null) { throw new ValidationMetadataException( - "ValidateSetGeneratedValidValuesListIsEmpty", + "ValidateSetGeneratedValidValuesListIsNull", null, - Metadata.ValidateSetGeneratedValidValuesListIsEmpty); + Metadata.ValidateSetGeneratedValidValuesListIsNull); } return validValuesLocal; diff --git a/src/System.Management.Automation/resources/Metadata.resx b/src/System.Management.Automation/resources/Metadata.resx index a6144717d6c..d223e3d2944 100644 --- a/src/System.Management.Automation/resources/Metadata.resx +++ b/src/System.Management.Automation/resources/Metadata.resx @@ -168,7 +168,7 @@ The argument "{0}" does not belong to the set "{1}" specified by the ValidateSet attribute. Supply an argument that is in the set and then try the command again. - + Valid values generator return a null value. diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index 248ed1dd867..0a91c64ffb7 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -307,7 +307,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { $exc = { Get-TestValidateSet5 -Param1 "TestString1" -ErrorAction Stop } | ShouldBeErrorId "ParameterArgumentValidationError,Test.Language.TestValidateSetCommand5" - $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsNull" } } @@ -395,7 +395,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { $exc = { Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction Stop } | ShouldBeErrorId "ParameterArgumentValidationError,Get-TestValidateSetPS5" - $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsEmpty" + $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should BeExactly "ValidateSetGeneratedValidValuesListIsNull" } It 'Unimplemented valid values generator type throws in PowerShell script' { @@ -465,7 +465,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { Get-TestValidateSetPS4 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" } - It 'Can implement CachedValidValuesGeneratorBase in PowerShell' { + It 'Can implement CachedValidValuesGeneratorBase with cache expiration in PowerShell' { Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" Start-Sleep 2 From e27daba81f2525c26898a8e3f42405eff093ee19 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 29 Jun 2017 11:08:23 +0300 Subject: [PATCH 13/20] Reuse TypeNotFound --- .../engine/parser/Compiler.cs | 20 +++++++++---------- .../resources/ParserStrings.resx | 3 --- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index bb0b4a5ccb0..d53fcf2d00f 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -1311,7 +1311,7 @@ private static Attribute NewValidateSetAttribute(AttributeAst ast) else { throw InterpreterError.NewInterpreterException(ast, typeof(RuntimeException), ast.Extent, - "CustomValidValuesGeneratorTypeNotFound", ParserStrings.CustomValidValuesGeneratorTypeNotFound, generatorTypeAst.TypeName.FullName, typeof(System.Management.Automation.IValidateSetValuesGenerator).FullName); + "TypeNotFound", ParserStrings.TypeNotFound, generatorTypeAst.TypeName.FullName, typeof(System.Management.Automation.IValidateSetValuesGenerator).FullName); } } else @@ -1377,7 +1377,7 @@ internal static Attribute GetAttribute(AttributeAst attributeAst) if (attributeType == null) { throw InterpreterError.NewInterpreterException(attributeAst, typeof(RuntimeException), attributeAst.Extent, - "CustomAttributeTypeNotFound", ParserStrings.CustomAttributeTypeNotFound, attributeAst.TypeName.FullName); + "TypeNotFound", ParserStrings.TypeNotFound, attributeAst.TypeName.FullName); } Func generator; @@ -3018,7 +3018,7 @@ public object VisitPipeline(PipelineAst pipelineAst) { var pipeElements = pipelineAst.PipelineElements; var firstCommandExpr = (pipeElements[0] as CommandExpressionAst); - + if (firstCommandExpr != null && pipeElements.Count == 1) { if (firstCommandExpr.Redirections.Count > 0) @@ -3034,7 +3034,7 @@ public object VisitPipeline(PipelineAst pipelineAst) { Expression input; int i, commandsInPipe; - + if (firstCommandExpr != null) { if (firstCommandExpr.Redirections.Count > 0) @@ -3055,7 +3055,7 @@ public object VisitPipeline(PipelineAst pipelineAst) // here so that we can tell the difference b/w $null and no input when // starting the pipeline, in other words, PipelineOps.InvokePipe will // not pass this value to the pipe. - + input = ExpressionCache.AutomationNullConstant; i = 0; commandsInPipe = pipeElements.Count; @@ -3063,16 +3063,16 @@ public object VisitPipeline(PipelineAst pipelineAst) Expression[] pipelineExprs = new Expression[commandsInPipe]; CommandBaseAst[] pipeElementAsts = new CommandBaseAst[commandsInPipe]; var commandRedirections = new object[commandsInPipe]; - + for (int j = 0; i < pipeElements.Count; ++i, ++j) { var pipeElement = pipeElements[i]; pipelineExprs[j] = Compile(pipeElement); - + commandRedirections[j] = GetCommandRedirections(pipeElement); pipeElementAsts[j] = pipeElement; } - + // The redirections are passed as a CommandRedirection[][] - one dimension for each command in the pipe, // one dimension because each command may have multiple redirections. Here we create the array for // each command in the pipe, either a compile time constant or created at runtime if necessary. @@ -3096,7 +3096,7 @@ public object VisitPipeline(PipelineAst pipelineAst) // No redirections. redirectionExpr = ExpressionCache.NullCommandRedirections; } - + if (firstCommandExpr != null) { var inputTemp = Expression.Variable(input.Type); @@ -3104,7 +3104,7 @@ public object VisitPipeline(PipelineAst pipelineAst) exprs.Add(Expression.Assign(inputTemp, input)); input = inputTemp; } - + Expression invokePipe = Expression.Call( CachedReflectionInfo.PipelineOps_InvokePipeline, input.Cast(typeof(object)), diff --git a/src/System.Management.Automation/resources/ParserStrings.resx b/src/System.Management.Automation/resources/ParserStrings.resx index db0d3a73511..4390b762e87 100644 --- a/src/System.Management.Automation/resources/ParserStrings.resx +++ b/src/System.Management.Automation/resources/ParserStrings.resx @@ -759,9 +759,6 @@ Possible matches are Cannot find the type for custom attribute '{0}'. Make sure that the assembly that contains this type is loaded. - - Cannot find the valid values generator type with name '{0}'. Make sure that the type is defined and implement '{1}' interface. - Cannot find an appropriate constructor to instantiate the custom attribute object for type '{0}'. From 18a791c71f29ebc3c95673dc0e9b3e65a35de533 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 29 Jun 2017 14:41:38 +0300 Subject: [PATCH 14/20] Remove `validValuesGeneratorType`. Add `CachedValidValuesGeneratorBase` constructor --- .../engine/Attributes.cs | 49 +++++++++---------- .../Scripting.Classes.Attributes.Tests.ps1 | 8 +-- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index bcb9c697f9c..a368dfd8790 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1365,18 +1365,12 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat private string[] _validValues; /// - /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. - /// By default 'ValidValuesCacheExpiration = 0' and no valid values is cached (is re-evaluated every time it is used). + /// Initializes a new instance of the CachedValidValuesGeneratorBase class. /// - protected int ValidValuesCacheExpiration { - get - { - return _validValuesCacheExpiration / 1000; - } - set - { - _validValuesCacheExpiration = value * 1000; - } + /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. + public CachedValidValuesGeneratorBase(int cacheExpirationInSeconds) + { + _validValuesCacheExpiration = cacheExpirationInSeconds; } private int _validValuesCacheExpiration; @@ -1411,7 +1405,7 @@ public string[] GetValidValues() if (_validValuesCacheExpiration > 0) { - Task.Delay(_validValuesCacheExpiration).ContinueWith((task) => _validValues = null); + Task.Delay(_validValuesCacheExpiration * 1000).ContinueWith((task) => _validValues = null); _validValues = validValuesNoCache; } @@ -1426,14 +1420,13 @@ public string[] GetValidValues() public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute { // We can use either static '_validValues' - // or dynamic valid values list generated by instance of 'validValuesGeneratorType'. + // or dynamic valid values list generated by instance of 'validValuesGenerator'. private string[] _validValues; + private IValidateSetValuesGenerator validValuesGenerator = null; // The valid values generator cache works across 'ValidateSetAttribute' instances. private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); - private Type validValuesGeneratorType = null; - /// /// Gets or sets the custom error message that is displayed to the user /// @@ -1453,21 +1446,18 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute public bool IgnoreCase { get; set; } = true; /// - /// Gets the values in the set + /// Gets the valid values in the set. /// public IList ValidValues { get { - if (validValuesGeneratorType == null) + if (validValuesGenerator == null) { return _validValues; } - IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry; - s_ValidValuesGeneratorCache.TryGetValue(validValuesGeneratorType, out ValidValuesGeneratorCacheEntry); - - var validValuesLocal = ValidValuesGeneratorCacheEntry?.GetValidValues(); + var validValuesLocal = validValuesGenerator.GetValidValues(); if (validValuesLocal == null) { @@ -1557,13 +1547,18 @@ public ValidateSetAttribute(Type valuesGeneratorType) throw PSTraceSource.NewArgumentException("valuesGeneratorType"); } - validValuesGeneratorType = valuesGeneratorType; + s_ValidValuesGeneratorCache.TryGetValue(valuesGeneratorType, out IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry); + + if (ValidValuesGeneratorCacheEntry == null) + { + // Add a valid values generator to the cache. + // We don't cache valid values. + // We expect that valid values can be cached in the valid values generator. + ValidValuesGeneratorCacheEntry = (IValidateSetValuesGenerator)Activator.CreateInstance(valuesGeneratorType); + s_ValidValuesGeneratorCache.TryAdd(valuesGeneratorType, ValidValuesGeneratorCacheEntry); + } - // Add a valid values generator to the cache. - // We don't cache valid values. - // We expect that valid values can be cached in the valid values generator. - var ValidValuesGeneratorCacheEntry = (IValidateSetValuesGenerator)Activator.CreateInstance(validValuesGeneratorType); - s_ValidValuesGeneratorCache.TryAdd(validValuesGeneratorType, ValidValuesGeneratorCacheEntry); + validValuesGenerator = ValidValuesGeneratorCacheEntry; } } diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index 0a91c64ffb7..47ed93da19e 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -401,7 +401,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { It 'Unimplemented valid values generator type throws in PowerShell script' { { Get-TestValidateSetPS6 -Param1 "AnyTestString" -ErrorAction Stop - } | ShouldBeErrorId "CustomValidValuesGeneratorTypeNotFound" + } | ShouldBeErrorId "TypeNotFound" } } @@ -409,6 +409,9 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { BeforeAll { class GenValuesForParam : System.Management.Automation.CachedValidValuesGeneratorBase { + GenValuesForParam() : base(300) { + } + [String[]] GenerateValidValues() { return [string[]]("Test1","TestString1","Test2") @@ -416,8 +419,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { } class GenValuesWithExpiration : System.Management.Automation.CachedValidValuesGeneratorBase { - GenValuesWithExpiration() : base() { - $this.ValidValuesCacheExpiration = 2 + GenValuesWithExpiration() : base(2) { } Static [bool] $temp = $true; From 1319aef43b2cdd2cac27eaddc9df4d87606b5813 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 29 Jun 2017 15:42:28 +0300 Subject: [PATCH 15/20] Add 'ClearValidValuesGeneratorCache' --- src/System.Management.Automation/engine/Attributes.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index a368dfd8790..9f18cd43dd7 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1427,6 +1427,14 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute // The valid values generator cache works across 'ValidateSetAttribute' instances. private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); + /// + /// Clear the valid values generator cache. + /// + public static void ClearValidValuesGeneratorCache() + { + s_ValidValuesGeneratorCache.Clear(); + } + /// /// Gets or sets the custom error message that is displayed to the user /// From a951f7f8257aaf4fbba5ae94f72aa12e2b3dd2b6 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 30 Jun 2017 06:57:28 +0300 Subject: [PATCH 16/20] Remove `ClearValidValuesGeneratorCache` --- src/System.Management.Automation/engine/Attributes.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 9f18cd43dd7..8832f9e42ea 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1405,8 +1405,8 @@ public string[] GetValidValues() if (_validValuesCacheExpiration > 0) { - Task.Delay(_validValuesCacheExpiration * 1000).ContinueWith((task) => _validValues = null); _validValues = validValuesNoCache; + Task.Delay(_validValuesCacheExpiration * 1000).ContinueWith((task) => _validValues = null); } return validValuesNoCache; @@ -1427,14 +1427,6 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute // The valid values generator cache works across 'ValidateSetAttribute' instances. private static ConcurrentDictionary s_ValidValuesGeneratorCache = new ConcurrentDictionary(); - /// - /// Clear the valid values generator cache. - /// - public static void ClearValidValuesGeneratorCache() - { - s_ValidValuesGeneratorCache.Clear(); - } - /// /// Gets or sets the custom error message that is displayed to the user /// From f8ba27e77d014d09dd713f9134608af1a1b0b1e9 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Tue, 11 Jul 2017 08:43:45 +0300 Subject: [PATCH 17/20] Make constructor protected. Fix 'if'. --- src/System.Management.Automation/engine/Attributes.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 8832f9e42ea..3305223f7ef 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1363,18 +1363,17 @@ public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerat { // Cached valid values. private string[] _validValues; + private int _validValuesCacheExpiration; /// /// Initializes a new instance of the CachedValidValuesGeneratorBase class. /// /// Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache. - public CachedValidValuesGeneratorBase(int cacheExpirationInSeconds) + protected CachedValidValuesGeneratorBase(int cacheExpirationInSeconds) { _validValuesCacheExpiration = cacheExpirationInSeconds; } - private int _validValuesCacheExpiration; - /// /// Abstract method to generate a valid values. /// @@ -1547,9 +1546,7 @@ public ValidateSetAttribute(Type valuesGeneratorType) throw PSTraceSource.NewArgumentException("valuesGeneratorType"); } - s_ValidValuesGeneratorCache.TryGetValue(valuesGeneratorType, out IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry); - - if (ValidValuesGeneratorCacheEntry == null) + if (!s_ValidValuesGeneratorCache.TryGetValue(valuesGeneratorType, out IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry)) { // Add a valid values generator to the cache. // We don't cache valid values. From 7ff271f66eb3d3dc0f2c3afa2ed51053746b5bf0 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Wed, 12 Jul 2017 07:10:02 +0300 Subject: [PATCH 18/20] Add a check Type.IsNonPublic. Replace 'if' with GetOrAdd() --- .../engine/Attributes.cs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index 3305223f7ef..fbf200571a6 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -1541,21 +1541,16 @@ public ValidateSetAttribute(params string[] validValues) /// for null arguments public ValidateSetAttribute(Type valuesGeneratorType) { - if (!typeof(IValidateSetValuesGenerator).IsAssignableFrom(valuesGeneratorType)) + // We check 'IsNotPublic' because we don't want allow 'Activator.CreateInstance' create an instance of non-public type. + if (!typeof(IValidateSetValuesGenerator).IsAssignableFrom(valuesGeneratorType) || valuesGeneratorType.IsNotPublic) { throw PSTraceSource.NewArgumentException("valuesGeneratorType"); } - if (!s_ValidValuesGeneratorCache.TryGetValue(valuesGeneratorType, out IValidateSetValuesGenerator ValidValuesGeneratorCacheEntry)) - { - // Add a valid values generator to the cache. - // We don't cache valid values. - // We expect that valid values can be cached in the valid values generator. - ValidValuesGeneratorCacheEntry = (IValidateSetValuesGenerator)Activator.CreateInstance(valuesGeneratorType); - s_ValidValuesGeneratorCache.TryAdd(valuesGeneratorType, ValidValuesGeneratorCacheEntry); - } - - validValuesGenerator = ValidValuesGeneratorCacheEntry; + // Add a valid values generator to the cache. + // We don't cache valid values. + // We expect that valid values can be cached in the valid values generator. + validValuesGenerator = s_ValidValuesGeneratorCache.GetOrAdd(valuesGeneratorType, (key) => (IValidateSetValuesGenerator)Activator.CreateInstance(key)); } } From 8b992a99bdecafdbd64e18943d5c5a32eb6ba4f4 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 13 Jul 2017 11:52:39 +0300 Subject: [PATCH 19/20] Revert exception. --- src/System.Management.Automation/engine/parser/Compiler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index d53fcf2d00f..9972e7ff1f8 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -1377,7 +1377,7 @@ internal static Attribute GetAttribute(AttributeAst attributeAst) if (attributeType == null) { throw InterpreterError.NewInterpreterException(attributeAst, typeof(RuntimeException), attributeAst.Extent, - "TypeNotFound", ParserStrings.TypeNotFound, attributeAst.TypeName.FullName); + "CustomAttributeTypeNotFound", ParserStrings.CustomAttributeTypeNotFound, attributeAst.TypeName.FullName); } Func generator; From 65349de2b25022092213d86cfc1f5dfe0c92cbc6 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 13 Jul 2017 13:51:59 +0300 Subject: [PATCH 20/20] Fix test --- .../Language/Classes/Scripting.Classes.Attributes.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 index 47ed93da19e..d378a1abfbd 100644 --- a/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 +++ b/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 @@ -470,7 +470,7 @@ Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { It 'Can implement CachedValidValuesGeneratorBase with cache expiration in PowerShell' { Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" Get-TestValidateSetPS5 -Param1 "TestString1" -ErrorAction SilentlyContinue | Should BeExactly "TestString1" - Start-Sleep 2 + Start-Sleep 3 Get-TestValidateSetPS5 -Param1 "TestString2" -ErrorAction SilentlyContinue | Should BeExactly "TestString2" }