Allow PATCH objects with nullable attribute (e.g. DateTime?) #96
Conversation
| @@ -26,10 +26,15 @@ public void SetValue(object entity, object newValue) | |||
| var propertyInfo = entity | |||
| .GetType() | |||
| .GetProperty(InternalAttributeName); | |||
|
|
|||
| var convertedValue = Convert.ChangeType(newValue, propertyInfo.PropertyType); | |||
There was a problem hiding this comment.
Thanks for the PR!
We do have a method called TypeHelper.ConvertType which is where type conversions should be processed, but looks like I missed this here. Can you try using that method instead and see if your tests still pass?
There was a problem hiding this comment.
Hi @jaredcnance, I tested with TypeHelper.ConvertType and all tests are green!
While reading this method, I was wondering if it was not a good idea to replace
if (type.GetTypeInfo().IsGenericType && type.GetGenericTypeDefinition().Equals(typeof(Nullable<>)))
type = Nullable.GetUnderlyingType(type);
by
type = Nullable.GetUnderlyingType(type) ?? type;
It changes nothing about the result (I think), but it is more easy to read (maybe subjective), WDYT?
Thanks a lot for review!
There was a problem hiding this comment.
Yeah that looks fine to me and I agree the intent is clearer. Thanks again for the PR!
| var convertedValue = (newValue == null) ? null : Convert.ChangeType(newValue, t); | ||
|
|
||
| propertyInfo.SetValue(entity, convertedValue, null); | ||
| } |
There was a problem hiding this comment.
some funkiness with your tab spacing throughout...not a big deal just a note
Use TypeHelper.ConvertType to keep consistency.
Fix use spaces instead of tabs.
Closes #95.
Ready to merge, tests are running.
Problem was located in the AttrAttribute class, while setting the new value on model attributes. It was crashing on nullable types (e.g DateTime?).
Fixed as http://stackoverflow.com/questions/3531318/convert-changetype-fails-on-nullable-types.
Thank you!