4

Closed

System.Web.Http.OData: HTTP PATCH with Delta<T> should allow more than just basic objects for properties.

description

This post focuses on the functionality of the TrySetPropertyValue method of the Delta class.

When doing an http PATCH with the (very cool) Delta class, one can only update properties that are basic types or classes. Inherited classes, Nullables and Generics aren't supported.

As far as I can tell, this is due to the type checking that happens in the TrySetPropertyValue(_name, _value) method of the Delta class. If the serialized _value object (in my case from Json) doesn't match exactly the type of the _name property, the method will exit and not set the property.

This is why it doesn't work with e.g. ICollection<T> or any derived classes.

Another type-related issue (this is maybe a bug and not a feature request) is that the Json parser converts integers to 64 bit ones, which won't match the 32 bit int properties declared on the entity and thus get ignored by this method.
Also, Nullable types return false for .IsClass and thus can't be set to null according to the logic in the method.

I would suggest changing the method as such to fix these issues (might require further changes in the deserializer to support collection properties):
public bool TrySetPropertyValue(string name, object value)
{
  if (name == null)
  {
    throw Error.ArgumentNull("name");
  }

  if (!_propertiesThatExist.ContainsKey(name))
  {
    return false;
  }

  PropertyAccessor<TEntityType> cacheHit = _propertiesThatExist[name];
  Type valueType = value != null ? value.GetType() : null;
  // Check if the types differ and doesn't inherit
  if (cacheHit.Property.PropertyType != valueType
    && !cacheHit.Property.PropertyType.IsAssignableFrom(valueType))
  {
    if (HasConversionOperator(valueType, cacheHit.Property.PropertyType))
    {
      // try to cast to the correct type
      try
      {
        var sConvert = TypeDescriptor.GetConverter(valueType);
        string temp = sConvert.ConvertToString(value);
        var tConvert = TypeDescriptor.GetConverter(cacheHit.Property.PropertyType);
        value = tConvert.ConvertFromString(temp);
      }
      catch
      {
        return false;
      }
    }
    else if (!((cacheHit.Property.PropertyType.IsClass || Nullable.GetUnderlyingType(cacheHit.Property.PropertyType) != null) 
      && value == null))
    {
      // If you cannot cast and we're not setting it null, don't set the property
      return false;
    }
  }

  //.Setter.Invoke(_entity, new object[] { value });
  cacheHit.SetValue(_entity, value);
  _changedProperties.Add(name);
  return true;
}

... where HasConversionOperator is a function that checks if an object can be casted to another e.g. :
public static bool HasConversionOperator(Type from, Type to)
{
if (from == null)
  return false;
  Func<Expression, UnaryExpression> bodyFunction = body => Expression.Convert(body, to);
  ParameterExpression inp = Expression.Parameter(from, "inp");
  try
  {
    // If this succeeds then we can cast 'from' type to 'to' type using implicit coercion
    Expression.Lambda(bodyFunction(inp), inp).Compile();
    return true;
  }
  catch (InvalidOperationException)
  {
    return false;
  }
}
I converted to string and then to the target type, because some conversions aren't supported by TypeConverter e.g. : Int64 to Nullable<int>
Closed Jan 3, 2013 at 1:52 AM by hongyes
Verified to be fixed in latest build.

comments

hongyes wrote Oct 18, 2012 at 7:40 PM

I can repro the issues. It seems that we have some wrong logic in nullable case.
  1. Set null to nullable doesn't work
  2. Set int value to nullable<int> doesn't work
The integer issue works for interger property
  1. Set int value to int property works

blake05 wrote Nov 12, 2012 at 9:38 PM

I can reproduce this as well. I have a model that has basic properties like:
public class MyModel {
    public MyModel () {
        Keys = new HashSet<string>();
        Configuration = new Configuration();
    }

    public string Name { get; set; }
    public HashSet<string> Keys { get; set; }
    public Configuration Configuration { get; set; }
}


public class Configuration {
    public Configuration() {
        Settings = new ConfigurationDictionary();
    }

    public int Version { get; private set; }

    public ConfigurationDictionary Settings { get; set; }
}



public class ConfigurationDictionary : Dictionary<string, string> {}
If I try to submit json to any controller action that has a strongly typed delta parameter... Only simple types are picked up (E.G., No Dictionary, or HashSet).

var json = "{\"Name\":\"Test\",\"Keys\":[\"ad377f1f-e894-4ed0-9b73-f9f68c5e166c\",\"f21e0b1b-7592-4605-a059-916468187583\",\"5f6a5091-70fa-4130-903e-a6efee61da48\"],\"Configuration\":{\"Settings\":[{\"key\":\"test\",\"value\":\"test\"}]}}";

blake05 wrote Nov 12, 2012 at 9:41 PM

I can create a sample if needed.

raghuramn wrote Dec 5, 2012 at 2:05 AM

Fixed with http://aspnetwebstack.codeplex.com/SourceControl/changeset/4a1dd862fbce.

Also, note that Delta<T> works with only settable properties which might be an issue for collection properties as they are in general get-only. I have opened issue 670 to track it.

grennis wrote Feb 19, 2013 at 4:15 AM

I do not see this fixed in v4.0. I still cannot use Patch with Delta<> with an integer property. It does not appear in the list of changed property names.

grennis wrote Feb 19, 2013 at 4:20 AM

To clarify my comment, if I define the property as Int64, it works. But using int it does not work. The original bug mentioned this but it seems to have been overlooked in the fix.

raghuramn wrote Feb 19, 2013 at 6:10 PM

@grennis: Are you using the ODataMediaTypeFormatter? Delta<T> is only built for using along with ODataMediaTypeFormatter.