Skip to content

ValidateSetAttribute enhancement: support set values to be dynamically generated from a custom ValidateSetValueGenerator #3784

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Jul 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 119 additions & 11 deletions src/System.Management.Automation/engine/Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@

using System.Collections;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Text.RegularExpressions;
using System.Globalization;
using System.Management.Automation.Internal;
using System.Diagnostics.CodeAnalysis;
using System.Management.Automation.Language;
using System.Runtime.CompilerServices;
using System.Linq;
using System.Threading.Tasks;

namespace System.Management.Automation.Internal
{
Expand Down Expand Up @@ -1115,7 +1118,7 @@ public sealed class ValidatePatternAttribute : ValidateEnumeratedArgumentsAttrib

/// <summary>
/// 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.
/// <example>
Expand Down Expand Up @@ -1177,10 +1180,10 @@ public sealed class ValidateScriptAttribute : ValidateEnumeratedArgumentsAttribu
{
/// <summary>
/// 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.
///
///
/// <example>
/// [ValidateScript("$_ % 2", ErrorMessage = "The item '{0}' did not pass validation of script '{1}'")]
/// </example>
Expand Down Expand Up @@ -1353,20 +1356,82 @@ public ValidateCountAttribute(int minLength, int maxLength)
}
}

/// <summary>
/// Optional base class for <see cref="IValidateSetValuesGenerator"/> implementations that want a default implementation to cache valid values.
/// </summary>
public abstract class CachedValidValuesGeneratorBase : IValidateSetValuesGenerator
{
// Cached valid values.
private string[] _validValues;
private int _validValuesCacheExpiration;

/// <summary>
/// Initializes a new instance of the CachedValidValuesGeneratorBase class.
/// </summary>
/// <param name="cacheExpirationInSeconds">Sets a time interval in seconds to reset the '_validValues' dynamic valid values cache.</param>
protected CachedValidValuesGeneratorBase(int cacheExpirationInSeconds)
{
_validValuesCacheExpiration = cacheExpirationInSeconds;
}

/// <summary>
/// Abstract method to generate a valid values.
/// </summary>
public abstract string[] GenerateValidValues();

/// <summary>
/// Get a valid values.
/// </summary>
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.
var validValuesLocal = _validValues;
if (validValuesLocal != null)
{
return validValuesLocal;
}

var validValuesNoCache = GenerateValidValues();

if (validValuesNoCache == null)
{
throw new ValidationMetadataException(
"ValidateSetGeneratedValidValuesListIsNull",
null,
Metadata.ValidateSetGeneratedValidValuesListIsNull);
}

if (_validValuesCacheExpiration > 0)
{
_validValues = validValuesNoCache;
Task.Delay(_validValuesCacheExpiration * 1000).ContinueWith((task) => _validValues = null);
}

return validValuesNoCache;
}
}

/// <summary>
/// Validates that each parameter argument is present in a specified set
/// </summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute
{
// We can use either static '_validValues'
// 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<Type, IValidateSetValuesGenerator> s_ValidValuesGeneratorCache = new ConcurrentDictionary<Type, IValidateSetValuesGenerator>();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it matters, but I still want to bring it up -- this static dictionary will hold references to the instances of IValidateSetValuesGenerator and cause them to not be collected forever. Maybe there won't be too many IValidateSetValuesGenerator instances anyways, but this is still a potential memory leaking problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Above I tried to add cleanup but @lzybkr concluded that it didn't matter - we don't expect a huge number of generators.

Copy link
Member

Choose a reason for hiding this comment

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

#Closed


/// <summary>
/// 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.
///
///
/// <example>
/// [ValidateSet("A","B","C", ErrorMessage="The item '{0}' is not part of the set '{1}'.")
/// </example>
Expand All @@ -1380,13 +1445,28 @@ public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute
public bool IgnoreCase { get; set; } = true;

/// <summary>
/// Gets the values in the set
/// Gets the valid values in the set.
/// </summary>
public IList<string> ValidValues
{
get
{
return _validValues;
if (validValuesGenerator == null)
{
return _validValues;
}

var validValuesLocal = validValuesGenerator.GetValidValues();
Copy link
Member

Choose a reason for hiding this comment

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

Exception may be thrown from GetValidValues(). Shall we catch all exceptions and wrap them into a ValidationMetadataException? My concern is about how ParameterBinder will deal with the exceptions if we don't catch it here. I believe the parameter binder will handle any exceptions thrown during the binding, but maybe it's still better if we handle it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems related #3769 - completers silently falls back to paths.
I tested a generator with throw "Test throw!" - parameter binder handles any exceptions:

Get-TestValidateSetPS4 : Cannot validate argument on parameter 'Param1'. Test throw!
At line:1 char:32
+ Get-TestValidateSetPS4 -param1 fdfd
+                                ~~~~
    + CategoryInfo          : InvalidData: (:) [Get-TestValidateSetPS4], ParameterBindingValidationException
    + FullyQualifiedErrorId : ParameterArgumentValidationError,Get-TestValidateSetPS4

If the generator throws good formatted exception users get it in InnerException.
So it seems redundant to add extra level to catch the exception.

Copy link
Member

@daxian-dbw daxian-dbw Jul 11, 2017

Choose a reason for hiding this comment

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

#Close. Agreed that wrapping the exception is not necessary.


if (validValuesLocal == null)
{
throw new ValidationMetadataException(
"ValidateSetGeneratedValidValuesListIsNull",
null,
Metadata.ValidateSetGeneratedValidValuesListIsNull);
}

return validValuesLocal;
}
}

Expand All @@ -1409,16 +1489,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;
}
Expand Down Expand Up @@ -1455,6 +1532,37 @@ public ValidateSetAttribute(params string[] validValues)

_validValues = validValues;
}

/// <summary>
/// Initializes a new instance of the ValidateSetAttribute class.
/// Valid values is returned dynamically from a custom class implementing 'IValidateSetValuesGenerator' interface.
/// </summary>
/// <param name="valuesGeneratorType">class that implements the 'IValidateSetValuesGenerator' interface</param>
/// <exception cref="ArgumentException">for null arguments</exception>
public ValidateSetAttribute(Type 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");
}

// 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));
}
}

/// <summary>
/// Allows dynamically generate set of values for ValidateSetAttribute.
/// </summary>
public interface IValidateSetValuesGenerator
{
/// <summary>
/// Get a valid values.
/// </summary>
string[] GetValidValues();
}

#region Allow
Expand Down
46 changes: 33 additions & 13 deletions src/System.Management.Automation/engine/parser/Compiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1298,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,
"TypeNotFound", ParserStrings.TypeNotFound, 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);
Expand Down Expand Up @@ -2998,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)
Expand All @@ -3014,7 +3034,7 @@ public object VisitPipeline(PipelineAst pipelineAst)
{
Expression input;
int i, commandsInPipe;

if (firstCommandExpr != null)
{
if (firstCommandExpr.Redirections.Count > 0)
Expand All @@ -3035,24 +3055,24 @@ 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;
}
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.
Expand All @@ -3076,15 +3096,15 @@ public object VisitPipeline(PipelineAst pipelineAst)
// No redirections.
redirectionExpr = ExpressionCache.NullCommandRedirections;
}

if (firstCommandExpr != null)
{
var inputTemp = Expression.Variable(input.Type);
temps.Add(inputTemp);
exprs.Add(Expression.Assign(inputTemp, input));
input = inputTemp;
}

Expression invokePipe = Expression.Call(
CachedReflectionInfo.PipelineOps_InvokePipeline,
input.Cast(typeof(object)),
Expand Down
3 changes: 3 additions & 0 deletions src/System.Management.Automation/resources/Metadata.resx
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@
<data name="ValidateSetFailure" xml:space="preserve">
<value>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.</value>
</data>
<data name="ValidateSetGeneratedValidValuesListIsNull" xml:space="preserve">
<value>Valid values generator return a null value.</value>
</data>
<data name="ValidateFailureResult" xml:space="preserve">
<value>"{0}" failed on property "{1}" {2}</value>
</data>
Expand Down
Loading