Fix DefaultBinder conversions from Dictionary<string,object> #2052

joaompneves · 2017-05-17T14:31:35Z

The following example will work and not crash:

binder = new DefaultBinder(new DefaultFieldNameConverter());
obj = new Dictionary<string, object> { { "key", "value" } };
binder.Bind(new [] {  obj }, typeof(Dictionary<string, object>[])); => returns [ obj ]
binder.Bind(obj, typeof(Dictionary<string, object>)); => returns obj

Dictionaries were wrongly considered collections.
Some conversions involving Dictionaries caused NullReferenceExceptions to be thrown

…The following example will work and not crash: binder = new DefaultBinder(new DefaultFieldNameConverter()); result = new[] { new Dictionary<string, object> { { "key", "value" } } }; binder.Bind(result, typeof(Dictionary<string, object>[]));

amaitland · 2017-05-18T00:05:45Z

What's the go here? Is there actually a bug?

joaompneves · 2017-05-18T08:21:55Z

There is but I have to take a deep look at it, that's why I closed it for now. I'll re-open later.

…The following example will work and not crash: binder = new DefaultBinder(new DefaultFieldNameConverter()); result = new[] { new Dictionary<string, object> { { "key", "value" } } }; binder.Bind(result, typeof(Dictionary<string, object>[]));

amaitland

I think there is a cleaner way to resolve this, comments made inline.

Before this can be merged we'll need to add some code examples/use cases to BindingTest.html. Unit Tests probably should be added......

amaitland · 2017-05-22T04:22:11Z

CefSharp/ModelBinding/DefaultBinder.cs

@@ -72,6 +72,10 @@ public virtual object Bind(object obj, Type modelType)
                {
                    throw new ArgumentException("When modelType is an enumerable it must specify the type.", "modelType");
                }
+            } 
+            else if (modelType.IsGenericObject())


Type checking here seems a little weak, I think we'd be better off immediately after the obj == null check to using something like

//If the object can be directly assigned to the modelType then return immediately. if(modelType.IsAssignableFrom(obj.GetType())) { return obj; }

amaitland · 2017-05-22T04:24:19Z

CefSharp/ModelBinding/ModelBindingExtensions.cs

+        /// </summary>
+        /// <param name="source">source type</param>
+        /// <returns>return true if is a generic js object</returns>
+        public static bool IsGenericObject(this Type source) 


The name of this method is confusing, I look at it and think of .Net generics, I don't actually think it's required either if we make a few other simplifications.

amaitland · 2017-05-22T04:24:33Z

CefSharp/ModelBinding/ModelBindingExtensions.cs

@@ -94,7 +103,7 @@ public static bool IsAssignableToGenericType(this Type givenType, Type genericTy
        /// </summary>
        /// <param name="source">The type to check.</param>
        /// <returns><see langword="true" /> if the type is an collection, otherwise <see langword="false" />.</returns>
-        public static bool IsCollection(this Type source)
+        private static bool IsCollection(this Type source)


Reason for making this private is?

amaitland · 2017-05-22T04:27:10Z

CefSharp/ModelBinding/DefaultBinder.cs

            //If the type is a dictionary and the PropertyType isn't then we'll bind.
-            if (obj.GetType() == dictionaryType && modelProperty.PropertyType != dictionaryType)
+            if (obj.GetType().IsGenericObject() && !modelProperty.PropertyType.IsGenericObject())


As the two if branches here are duplicated I think we can simplify the whole BindValue method. We should be able to simplify things quite a bit, using something like the following.

protected virtual void BindValue(BindingMemberInfo modelProperty, object obj, BindingContext context) { if(obj == null) { return; } if (modelProperty.PropertyType.IsAssignableFrom(obj.GetType())) { //Simply set the property modelProperty.SetValue(context.Model, obj); } else { //Cannot directly set the property attempt to bind var model = Bind(obj, modelProperty.PropertyType); modelProperty.SetValue(context.Model, model); } }

amaitland · 2017-05-22T04:27:29Z

CefSharp/ModelBinding/DefaultBinder.cs

@@ -190,7 +192,7 @@ protected virtual object CreateModel(Type modelType, Type genericType)

        protected virtual object GetValue(string propertyName, BindingContext context)
        {
-            if (context.Object.GetType() == typeof(Dictionary<string, object>))
+            if (context.Object.GetType().IsGenericObject())


Please revert this and remove IsGenericObject extension method.

amaitland · 2017-05-22T04:28:13Z

CefSharp/ModelBinding/ModelBindingExtensions.cs

@@ -67,7 +66,17 @@ public static bool IsArray(this Type source)
        /// <returns>return if collection, array or enumerable</returns>
        public static bool IsCollectionOrArray(this Type source)
        {
-            return source.IsCollection() || source.IsArray() || source.IsEnumerable();
+            return !source.IsGenericObject() && (source.IsCollection() || source.IsArray() || source.IsEnumerable());


I'm hopeful we can revert this change with the other changes I've proposed.

amaitland · 2017-05-22T04:30:00Z

CefSharp/ModelBinding/DefaultBinder.cs

@@ -92,7 +96,7 @@ public virtual object Bind(object obj, Type modelType)

                    if (val != null)
                    {
-                        if (val.GetType() == typeof(Dictionary<string, object>))
+                        if (val.GetType().IsGenericObject())


Please revert this and remove IsGenericObject extension method.

… - greatly simplifies the code Remove IsCollectionOrArray extension method, use the long hand - makes the code more readable. Switch to using Activator.CreateInstance directly Remove a large number of unused extensions in ModelBindingExtensions Alternate to #2052

amaitland · 2017-05-30T04:33:09Z

I've merged my proposed changes in commit 0d48e30

Code is greatly simplified. If your still experiencing a problem I believe the simplifications are a better starting point for any further bug fixes. Where possible please include a test case that reproduces your problem, would make my life a lot easier, thank you.

joaompneves · 2017-05-31T14:32:22Z

It works as expected, great!
Where should I add the test case?
In the PR description I added a snippet of code (a test case) that was crashing before.

joaompneves closed this May 17, 2017

joaompneves added 2 commits May 18, 2017 11:21

Fixed indentation

31d8fb4

joaompneves changed the title ~~Fix DefaultBinder conversions of Dictionary<string,object>[]~~ Fix DefaultBinder conversions from Dictionary<string,object> May 18, 2017

joaompneves reopened this May 18, 2017

amaitland requested changes May 22, 2017

View reviewed changes

amaitland added this to the 59.0.0 milestone May 22, 2017

amaitland added the not-ready-for-merge label May 22, 2017

amaitland closed this May 30, 2017

amaitland modified the milestones: 59.0.0, 62.0.0 Dec 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DefaultBinder conversions from Dictionary<string,object> #2052

Fix DefaultBinder conversions from Dictionary<string,object> #2052

joaompneves commented May 17, 2017 •

edited by perlun

amaitland commented May 18, 2017

joaompneves commented May 18, 2017

amaitland left a comment

amaitland May 22, 2017

amaitland May 22, 2017

amaitland May 22, 2017

amaitland May 22, 2017

amaitland May 22, 2017

amaitland May 22, 2017

amaitland May 22, 2017

amaitland commented May 30, 2017

joaompneves commented May 31, 2017

Fix DefaultBinder conversions from Dictionary<string,object> #2052

Fix DefaultBinder conversions from Dictionary<string,object> #2052

Conversation

joaompneves commented May 17, 2017 • edited by perlun

amaitland commented May 18, 2017

joaompneves commented May 18, 2017

amaitland left a comment

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland May 22, 2017

Choose a reason for hiding this comment

amaitland commented May 30, 2017

joaompneves commented May 31, 2017

joaompneves commented May 17, 2017 •

edited by perlun