Make variable interface suitable for dynamic value lookup
Closed this issue · 14 comments
Is your feature request related to a problem? Please describe.
I use many variables. Therefore I implement the IDictionary<string, object> interface to lookup variable values dynamically. This worked until version 2.2.0. As of version 2.2.0, the Property Count seems to be used, which cannot be implemented using dynamic value lookup.
Describe the solution you'd like
Instead of IDictionary<string, object>, a simplified interface suitable for dynamic value lookup could be used:
public interface IVariables
{
bool TryGetValue(string key, [MaybeNullWhen(false)] out object value);
}
@sryffel thank you for the issue. I am surprised that the recent changes would cause an issue like this, although I wasn't aware was a use case being used 😄.
I can see why TryGetValue won't be called but I can't tell when it would have previously been called as this code has not changed in some time: https://github.com/bijington/expressive/blob/main/Source/Expressive/Expressions/VariableExpression.cs
Given how the code currently exists you would need to implement/override the indexer property in order to achieve dynamic loading (I think). I can take a look deeper in to this and come back with a concrete example but this is the part that I think would need to be implemented: https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.idictionary-2.item?view=net-5.0#System_Collections_Generic_IDictionary_2_Item__0_
Out of interest are you able to supply any example of some code that was working pre 2.2.0 and is now no longer working to help speed up diagnosing any possible regression?
Further to my comment I actually think that you would also need to implement ContainsKey although both should be enforced through implementing the IDictionary<TKey, TValue> interface.
All that said I would be happy to migrate to calling TryGetValue which should in theory allow your code to work?
I will run some tests and see what I can come up with.
Thank you for your quick response.
Here is a simple example to reproduce the behavior:
dictionarytest.zip
Thank you for the example of it failing. I can see it working with v2.1.1 and then failing with v2.2.0 so I need to work out what has changed to cause this issue. Sorry about this.
The issue appears to have been worsened from the below commit:
More accurately this code:
if (variables != null)
{
variables = new Dictionary<string, object>(variables, this.context.StringComparer);
}
I say worsened because it was still possible to invoke this issue if you passed in the ExpressiveOptions.IgnoreCase setting. The old code looked like:
if (variables != null &&
this.options.HasFlag(ExpressiveOptions.IgnoreCase))
{
variables = new Dictionary<string, object>(variables, StringComparer.OrdinalIgnoreCase);
}
I can revert back to only setting the comparer when the IgnoreCase setting is supplied which will solve your issue in the short term however given that I now know of this use case I would like to investigate whether I can:
- Document a way for users to supply their own implementation of
IDictionary<TKey, TValue> - As a possible further enhancement supply a base implementation that could be used.
This would be a great solution. Thank you.
Regarding your first point I think that the interface IDictionary<string, object> has too many methods that must be implemented even though most are not used. While my interface IVariables above does not seem to be suitable, a better one might be:
public interface IVariables
{
bool ContainsKey(string key);
object this[string key] { get; }
IVariables Clone(StringComparer stringComparer);
}
This could be passed to a Evaluate(IVariables variables = null) method. For backwards compatibility, the method Evaluate(IDictionary<string, object> variables) could be preserved by an internal class that implements IVariables using the dictionary.
It may have to be something like that. I will have a play with some ideas. If it is going to be custom then it could just expose a single method to provide the value e.g.
object ProvideValue(string key, Context context)
Context to potentially let the implementation deal with the case sensitivity issue that my original code was trying to solve.
Out of interest why do you use this dynamic lookup instead of just supplying a dictionary in? Supplying a large dictionary should still perform well enough
The reason why I am unlikely to move away from IDictionary is that so much of the code depends on that part of the implementation it would require serious work. That said I think the below should work...
I haven't been able to check how well this might perform but this does appear to work:
public class DynamicVariableProvider : IDictionary<string, object>
{
private readonly HashSet<string> variableNames;
private readonly Func<string, object> valueProvider;
public DynamicVariableProvider(IEnumerable<string> variableNames, Func<string, object> valueProvider)
{
this.variableNames = new HashSet<string>(variableNames);
this.valueProvider = valueProvider;
}
public IEnumerator<KeyValuePair<string, object>> GetEnumerator() =>
this.variableNames.Select(name => new KeyValuePair<string, object>(name, this.valueProvider.Invoke(name)))
.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();
public void Add(KeyValuePair<string, object> item) => ThrowNotSupported();
public void Clear() => ThrowNotSupported();
public bool Contains(KeyValuePair<string, object> item) => ThrowNotSupported();
public void CopyTo(KeyValuePair<string, object>[] array, int arrayIndex) => ThrowNotSupported();
public bool Remove(KeyValuePair<string, object> item) => ThrowNotSupported();
public int Count => this.variableNames.Count;
public bool IsReadOnly => true;
public void Add(string key, object value) => ThrowNotSupported();
public bool ContainsKey(string key) => this.variableNames.Contains(key);
public bool Remove(string key) => ThrowNotSupported();
public bool TryGetValue(string key, out object value)
{
var containsKey = ContainsKey(key);
value = containsKey
? this.valueProvider.Invoke(key)
: null;
return containsKey;
}
public object this[string key]
{
get => this.valueProvider.Invoke(key);
set => ThrowNotSupported();
}
public ICollection<string> Keys => this.variableNames;
public ICollection<object> Values { get; } // What to do here?
private static bool ThrowNotSupported() => throw new NotSupportedException("This implementation is readonly.");
}
Then you can use it like:
var expression = new Expression("1+[a]", ExpressiveOptions.IgnoreCaseForParsing);
var variableProvider = new DynamicVariableProvider(expression.ReferencedVariables, name =>
{
if (name == "a")
{
return 2;
}
return null;
});
Assert.AreEqual(3, expression.Evaluate<int>(variableProvider));
I would like to do some more testing and possibly even some profiling but if this approach suits you then I would be happy to ship this DynamicVariableProvider class within the framework so you would only really need to care about the 'how to use it part'.
The DynamicVariableProvider would be a great addition that would simplify my code quite a bit ;)
Nonetheless, I would prefer it if cloning the entire dictionary could be avoided. This seems unnecessary, since the user could instead pass a dictionary with the correct StringComparer.
Yes I completely agree that if someone has already supplied the Dictionary with the appropriate StringComparer then this is completely unnecessary.
It's such a shame this isn't already exposed on the IDictionary interface.
I guess if I expose an interface like you have suggested then I could do away with it but I haven't been able to easily make that fit in to the current approach
@sryffel I am keen to explore this route further however I do need to get a version released. I propose that I will undo the cloning of the dictionary of the IgnoreCase flag is not set and release. Then I will focus on this straight after the release.
Removed from milestone 'Next release' to allow this issue to stay open. Note the initial issue has been resolved in release v2.3.0 but I would like to keep this open and follow through with further investigation on dynamic variable loading.
@sryffel I think I have a working solution now...
Expression will expose a new method: public object Evaluate(IVariableProvider variableProvider)
IVariableProvider pretty much matches your suggested approach but I have been able to simplify the required implementation further:
/// <summary>
/// Interface definition for providing variable values.
/// </summary>
public interface IVariableProvider
{
/// <summary>
/// Attempts to safely get the <paramref name="value"/> for the supplied <paramref name="variableName"/>.
/// </summary>
/// <param name="variableName">The name of the variable.</param>
/// <param name="value">The value of the variable or <b>null</b> if it does not exist.</param>
/// <returns>true if the variable exists, false otherwise.</returns>
bool TryGetValue(string variableName, out object value);
}
Then you could call it like:
var context = new Context(ExpressiveOptions.IgnoreCaseForParsing);
var expression = new Expression("1+[a]", context);
var variableProvider = new MockProvider();
Assert.AreEqual(3, expression.Evaluate(variableProvider));
private class MockProvider : IVariableProvider
{
public bool TryGetValue(string variableName, out object value)
{
value = variableName == "a" ? (object)2 : null;
return variableName == "a";
}
}
I have been able to wrap IVariableProvider internally so that it covers all the unpleasantness of having to implement IDictionary<TKey, TValue>. Also for the record included in this change, if you pass in a Dictionary<string, object> with the appropriate StringComparer set it should no longer clone the dictionaries contents.
The changes are at #94. Hopefully I can add in some extra unit tests around the new parts and then it should be pretty good to test out.