Skip to content
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

Fix DynamicIndex implementation #480

Merged
merged 23 commits into from
Feb 12, 2021
Merged

Fix DynamicIndex implementation #480

merged 23 commits into from
Feb 12, 2021

Conversation

StefH
Copy link
Collaborator

@StefH StefH commented Feb 2, 2021

No description provided.

@StefH StefH self-assigned this Feb 2, 2021
@StefH StefH added the bug label Feb 2, 2021
@StefH StefH changed the title DynamicIndex (wip...) DynamicIndex Feb 2, 2021
@StefH StefH changed the title DynamicIndex Fix DynamicIndex implementation Feb 10, 2021
@StefH
Copy link
Collaborator Author

StefH commented Feb 10, 2021

Hello @JonathanMagnan,

Can you or a developer please take a look at this PR ?

This PR solves issue #448 but in order to fix that I had to remove the DynamicIndex logic which was introduced in 1.1.8 and use the original implementation (with a small change) to make sure that #397 keeps working correctly.

For that, I did enable the unittest again:
https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/480/files#diff-8c64d2f034f0d47405c4723ad04680f885cd71c73a5b0f498d04e95949789348L95

Please review the changes.

@hazzik
Copy link
Contributor

hazzik commented Feb 11, 2021

@StefH with following implementation I was able to get a robust solution for my problem as well as un-comment ExpressionTests_Select_DynamicObjects test

internal class DynamicGetMemberBinder : GetMemberBinder
{
    private static readonly Type DynamicWrapperType = typeof(DynamicWrapper);
    private static readonly ConstructorInfo DynamicWrapperConstructor = DynamicWrapperType.GetConstructor(new[] {typeof(object)});
    private static readonly PropertyInfo DynamicWrapperTypeIndexer = DynamicWrapperType.GetProperty("Item");

    public DynamicGetMemberBinder(string name, [CanBeNull] ParsingConfig config) : base(name, !(config?.IsCaseSensitive == true))
    {
    }

    public override DynamicMetaObject FallbackGetMember(DynamicMetaObject target, DynamicMetaObject errorSuggestion)
    {
        var instance = Expression.New(DynamicWrapperConstructor, target.Expression);
        var indexExpression = Expression.MakeIndex(instance, DynamicWrapperTypeIndexer, new Expression[] {Expression.Constant(Name)});
        return DynamicMetaObject.Create(target.Value, indexExpression);
    }

    private class DynamicWrapper
    {
        private readonly object _item;
        private readonly Dictionary<string, PropertyInfo> _properties;

        public DynamicWrapper(object item)
        {
            _item = item;
            if (item != null && !(item is IDictionary<string, object>))
            {
                _properties = item.GetType().GetProperties()
                    .ToDictionary(p => p.Name, d => d);
            }
        }

        public object this[string value]
        {
            get
            {
                if (_item is null)
                    throw new NullReferenceException();
                
                if (_item is IDictionary<string, object> dictionary)
                    return dictionary[value];

                if (_properties.TryGetValue(value, out var property))
                    return property.GetValue(_item, null);

                throw new InvalidOperationException();
            }
        }
    }
}

@JonathanMagnan
Copy link
Member

Hello @StefH ,

The PR looks good to me.

@StefH StefH merged commit 618b3a0 into master Feb 12, 2021
@StefH StefH deleted the DynamicIndex branch February 12, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants