-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Conversation
public ValidateSetAttribute(Type type) | ||
{ | ||
IValidateSetValuesGenerator validValuesGenerator; | ||
if (typeof(IValidateSetValuesGenerator).IsAssignableFrom(type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the as
operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof(class) as IValidateSetValuesGenerator
don't work.
} | ||
else | ||
{ | ||
throw PSTraceSource.NewArgumentNullException("validValues"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut and paste? I would use ArgumentException
and please use the correct parameter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat confused that must be reported - thanks for clarify.
Fixed.
throw PSTraceSource.NewArgumentNullException("validValues"); | ||
} | ||
|
||
_validValues = validValuesGenerator.GetValidValues().ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't cache the values during construction - the set may change over time.
Also - we may want a way to query other bound parameters from the generator - which would not be possible in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I eliminated the caching.
Please clarify - do you say to put validValuesGenerator
in a property?
} | ||
} | ||
|
||
/// Implement of test IValidateSetValuesGenerator to return Null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation does not return null
- it returns an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend learning more about how the yield
keyword is implemented under the covers, e.g. this is a good article: http://csharpindepth.com/Articles/Chapter6/IteratorBlockImplementation.aspx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for useful link!
Test was removed.
@@ -216,3 +218,96 @@ Describe 'Type resolution with attributes' -Tag "CI" { | |||
} | |||
} | |||
} | |||
|
|||
Describe 'ValidateSet support a dynamically generated set' -Tag "CI" { | |||
Context 'C# test' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also provide a test implementing the interface in PowerShell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have in mind something like?
function Get-TestValidateSet3
{
[CmdletBinding()]
Param
(
[ValidateSet([type]::GetType("GenValuesForParam1"))]
$Param1
)
$Param1
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - I meant:
class GenValuesFromParam2 : IValidateSetValuesGenerator { ... }
...
[ValidateSet([GenValuesFromParam2])] ...
In other words - no Add-Type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following sample don't work - the type passed as string to public ValidateSetAttribute(params string[] validValues)
constructor - wrong binding 😕 :
class GenValuesForParam2 : System.Management.Automation.IValidateSetValuesGenerator {
[System.Collections.Generic.IEnumerable[String]] GetValidValues() { return "Test1","TestString","Test2" }
}
function Get-TestValidateSet3
{
[CmdletBinding()]
Param
(
[ValidateSet([GenValuesForParam2])]
$Param1
)
$Param1
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thought - some of the dynamic sets are expensive to compute, e.g. querying Azure or whatever.
One strategy is to cache the set for some time. Nothing in this design prevents such caching, but it would be better to implement the caching in PowerShell so that third party code can use it easily. Maybe that means adding a CacheExpiration
parameter that defaults to 0 - immediate expiration.
Still thinking out loud - the cache maybe belongs in the generator, but maybe we provide some helpers to manage it - perhaps as an abstract base class.
throw PSTraceSource.NewArgumentException("valuesGeneratorType"); | ||
} | ||
|
||
_validValues = validValuesGenerator.GetValidValues(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor should just save valuesGeneratorType
because we create just one instance of the attribute per use.
For example, if someone added ValidateSet
to the Name
parameter on Get-VM
- you would want the name of each vm whenever you called Get-VM
, not the names when you first called Get-VM
.
By constructing the instance and getting the values during validation, the set is dynamically generated when tab completion or validation is happening - which only aligns with construction once (for scripts, attributes are constructed at first invocation) or never (for binary cmdlets, attributes are eagerly constructed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -1359,7 +1360,7 @@ public ValidateCountAttribute(int minLength, int maxLength) | |||
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)] | |||
public sealed class ValidateSetAttribute : ValidateEnumeratedArgumentsAttribute | |||
{ | |||
private string[] _validValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't change, and I would add a new read-only property for the Type so you can tell the difference on which constructor was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -1386,7 +1387,7 @@ public IList<string> ValidValues | |||
{ | |||
get | |||
{ | |||
return _validValues; | |||
return _validValues.ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the dynamic case - this should probably return a new collection every time it's called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
256a341
to
cf8e2a8
Compare
Wouldn't that be too intrusive? |
I had feedback in the form of a PR to TabExpansionPlusPlus that caching should be built-in. |
Do you mean that the cache has to work between different |
Many cmdlets have parameters that are common, e.g. there are 72 commands in AzureRM.Compute that accept On my machine, it takes >1s to query the resource group names So I think a cache that works across |
It feels almost more often than not that I implement some sort of custom completion caching. But often with less than stellar aging/expiration. |
b11ca18
to
5a1a9d2
Compare
I had to rebase to get autoloading of helper modules. Global valid values cache was added. The only question is. I clean up this cache never. Should we add clean-up and where? Although it seems superfluous. |
5a1a9d2
to
aa03005
Compare
@lzybkr Could you please continue the code review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look pretty good.
I have a feeling that this will make many folks happy, but it will fall short for others, mostly around wanting access to some context, e.g. the cmdlet or some of the already bound parameters.
Hopefully we can get some feedback one way or the other on the usefulness of this as it is though.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be removed - it's an important optimization.
I think it should be straight forward to update NewValidateSetAttribute
.
If we did remove this line, we'd also want to remove the method as well - this was the only reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I temporary removed that to pass tests but I forgot to ask you about that.
Now fixed.
null, | ||
Metadata.ValidateSetGeneratedValidValuesListIsEmpty); | ||
} | ||
_validValues = validValuesCacheEntry.validValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason to store the values in the field instead of return validValuesCacheEntry.validValues;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would also be useful to clean up the cache - this could be done with something like Task.Delay(CacheExpiration).ContinueWith(() => validValuesCacheEntry.validValues = null);
This might not seem like a big deal, the collection should typically be small, but sometimes it might be large, so we should make the collection collectible if it's truly going to be used again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason to store the values in the field instead of return validValuesCacheEntry.validValues; ?
We only use this in SetAsString()
for error message. We could use ValidValues
if we're not afraid that there will be a cache expiration and a recalculation of ValidValues
and that there may be other data in the error message.
Task.Delay(CacheExpiration)
Different attributes can have different CacheExpiration
. How can we resolve the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please continue?
It seems we should clean up validValuesCache from expired Entries not validValuesCacheEntry.validValues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought I had replied.
I now think CacheExpiration
should not be an argument to the attribute because, as you point out, different uses might specify conflicting values. Instead, the generator should own the cache expiration policy, and the attribute should not cache the results from the generator.
I still like the idea of providing an abstract base class that implements a default caching policy - it would be optional for a generator to derive from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this to IValidateSetValuesGenerator interface?
And please comment my "It seems we should clean up validValuesCache from expired Entries (generators) ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, you could add CacheExpiration
to IValidateSetValuesGenerator
, but it would be something extra that some generators have no need for.
I agree we should clean up validValuesCache
- and ideally, I think that cache is optionally available to generators but not required. This is why I suggested an abstract base class that generators could derive from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lzybkr I still don't correct tests however please review my next step - Is that the right direction?
I removed valid values cache and CacheExpiration from ValidateSet attribute, enhance a generator cache (add cleanup - the idea is from CoreFX), add an abstract class for generators with valid values cache support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lzybkr Now tests adjusted.
Is this definitely limited to 'binary' cmdlets? Has anyone tested this using a PS declared class? |
@lfshr Scripts is supported - see tests in the PR. We will edit the PR description after completing this work. |
if (ValidValuesCacheExpiration > 0) | ||
{ | ||
Task.Delay(ValidValuesCacheExpiration).ContinueWith((task) => _validValues = null); | ||
_validValues = _validValuesNoCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter to Task.Delay
is in milliseconds, not seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider that the ValidValuesCacheExpiration
parameter is in milliseconds and add this in our documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I think that seconds is a more practical unit for the timeout.
return _validValues; | ||
} | ||
|
||
var _validValuesNoCache = GenerateValidValues(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_validValuesNoCache [](start = 16, length = 19)
Local variables should not use a _
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if (_validValues != null) | ||
{ | ||
return _validValues; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a race condition here.
The sequence to trigger the race is:
- _validValues != null
- The background task to release the cache runs, assigning _validValues with null.
- The return executes, returning null.
The fix is:
var validValuesLocal = _validValues;
if (validValuesLocal != null)
{
return validValuesLocal;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I thought about it, but I didn't know how to just fix it.
Fixed.
private string[] _validValues; | ||
|
||
// The valid values generator cache works across 'ValidateSetAttribute' instances. | ||
private static ConcurrentDictionary<Type, ValidValuesGeneratorCacheEntry> s_ValidValuesGeneratorCache = new ConcurrentDictionary<Type, ValidValuesGeneratorCacheEntry>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValidValuesGeneratorCacheEntry [](start = 50, length = 30)
I think this can be IValidateSetValuesGenerator
.
There will be very few types that implement this interface, so I think the extra complexity around cache cleanup on the generator is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this, we won't be able to override this generator. This can be useful for script generators. Otherwise, the script writer will have to restart PowerShell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused. The is cache cleanup not controlled by the user - so restarting PowerShell is a practical necessity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use ValidValuesGeneratorCacheExpiration
to implicitly remove expired values from the cache.
Main question is - will we be happy with the PowerShell restart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restarting PowerShell is an occasional annoyance, but I'm not sure this will make a difference.
For C# implementations, you need to restart anyway.
For script implementations - I think there are acceptable ways to develop your generator and test it without restarting PowerShell.
So my preference is to keep the code as simple as possible and only introduce additional complexity after gaining more experience with the feature and determining it really is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we can wait a feedback from users.
The cache is removed.
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd move creating the generator and storing it in the ConcurrentDictionary to the ValidateSet constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now the cache is removed and we can create the generator in the constructor.
Fixed.
} | ||
|
||
_validValues = ValidValuesGeneratorCacheEntry.validValuesGenerator.GetValidValues()?.ToArray(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we're always call ToArray
, maybe we should just change the interface to return string[]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to change public interface? Is the breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not being clear - I meant the interface introduced in this PR - IValidateSetValuesGenerator
- so not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - the more simple for scripts.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the failure in CI build is due to this. You can either change to base(1)
, or make it sleep for 3 seconds (not sure which is more reliable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not happy with the solution but set 3 seconds.
I think this is almost ready to be merged -- 2 more comments to be addressed.
I don't have the context of the |
@daxian-dbw
The idea is to pass a context to a valid value generator. |
/cc @joeyaiello We are near a Milstone - 996 repo forks :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I will merge this PR soon. I assume #3744 is fixed by this PR, so for further enhancement discussion, please open a new issue to continue. @iSazonov now that the design and implementation are complete, could you please update the PR description with a summary of the design and new functionalities? We need that information for the documentation. |
@daxian-dbw I added the PR description - please review. |
@iSazonov Perfect! Thank you for the great work! |
Many thanks @lzybkr for ideas and helps! |
Hi everyone, |
@Menelion Generally you should open a new issue for this kind of thing rather than comment on a closed PR you can reference a PR in an issue by doing You can find some documentation of the API here https://docs.microsoft.com/en-us/dotnet/api/system.management.automation.ivalidatesetvaluesgenerator?view=pscore-6.0.0 This features was added in PowerShell Core 6.0.0. To use, your PowerShell code must run in PowerShell Core 6.0.0 or newer. You can find example usage here in this PR or at https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1 |
@Menelion You could look our tests which was added in the PR. |
I am trying to use this, but I always get I am using the latest |
That interface was introduced in |
Close #3744
Currently
ValidateSetAttribute
accepts only explicit constants as valid values. This is a significant limitation. Sometimes we need to get valid values dynamically, ex., Azure VMs, logged-on users and so on. The PR add follow possibilities:IValidateSetValuesGenerator
interface to get valid values dynamically.CachedValidValuesGeneratorBase
abstract class to get valid values dynamically and cache the list to share with other ValidateSetAttribute attributes.We support valid values generators on PowerShell and C#.