Skip to content

Commit

Permalink
STJ: add proper support for property shadowing
Browse files Browse the repository at this point in the history
It is neccessary in order to support IGridViewDataSet type hierarchy.
The issue didn't occur before, because we didn't map interface correctly,
ignoring inherited properties. It didn't matter, because we never serialized them.
Now we can end up serializing interface if DynamicDispatch is disabled.

The shadowing allows redefining a property with another property of the same
.NET Name with a compatible type.
  • Loading branch information
exyi committed Apr 25, 2024
1 parent 1c70d4c commit 00e8a55
Show file tree
Hide file tree
Showing 7 changed files with 222 additions and 27 deletions.
6 changes: 3 additions & 3 deletions src/Framework/Core/ViewModel/BindAttribute.cs
Expand Up @@ -7,7 +7,7 @@ namespace DotVVM.Framework.ViewModel
/// <summary>
/// Specifies the binding direction.
/// </summary>
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
public class BindAttribute : Attribute
{

Expand All @@ -24,8 +24,8 @@ public class BindAttribute : Attribute
public bool? _allowDynamicDispatch;
/// <summary>
/// When true, DotVVM serializer will select the JSON converter based on the runtime type, instead of deciding it ahead of time.
/// This essentially enables serialization of properties defined derived types, but does not enable derive type deserialization.
/// By default, dynamic dispatch is enabled for abstract types (including interfaces).
/// This essentially enables serialization of properties defined derived types, but does not enable derive type deserialization, unless an instance of the correct type is prepopulated into the property.
/// By default, dynamic dispatch is enabled for abstract types (including interfaces and System.Object).
/// </summary>
public bool AllowDynamicDispatch { get => _allowDynamicDispatch ?? false; set => _allowDynamicDispatch = value; }

Expand Down
Expand Up @@ -3,18 +3,14 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using DotVVM.Framework.Configuration;
using System.Reflection;
using DotVVM.Framework.Utils;
using System.Security;
using System.Diagnostics;
using System.Text.Json;
using System.IO;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
using System.Security.Cryptography.X509Certificates;
using DotVVM.Framework.Controls;
using FastExpressionCompiler;

namespace DotVVM.Framework.ViewModel.Serialization
{
Expand Down Expand Up @@ -69,6 +65,8 @@ public class VMConverter<T>(ViewModelJsonConverter factory): JsonConverter<T>, I
{
if (state is null)
throw new ArgumentNullException(nameof(state), "DotvvmSerializationState must be created before calling the ViewModelJsonConverter.");
if (typeof(T) != typeToConvert)
throw new ArgumentException("typeToConvert must be the same as T", nameof(typeToConvert));

if (reader.TokenType == JsonTokenType.Null)
{
Expand Down Expand Up @@ -103,13 +101,13 @@ static void ReadObjectStart(ref Utf8JsonReader reader)
{
if (reader.TokenType == JsonTokenType.None) reader.Read();
if (reader.TokenType != JsonTokenType.StartObject)
throw new JsonException($"Expected StartObject token, but reader.TokenType={reader.TokenType}");
throw new JsonException($"Cannot deserialize '{typeof(T).ToCode()}': Expected StartObject token, but reader.TokenType = {reader.TokenType}");
reader.Read();
}
static void ReadEndObject(ref Utf8JsonReader reader)
{
if (reader.TokenType != JsonTokenType.EndObject)
throw new JsonException($"Expected EndObject token, but reader.TokenType={reader.TokenType}");
throw new JsonException($"Expected EndObject token, but reader.TokenType = {reader.TokenType}");
}

public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) =>
Expand Down
Expand Up @@ -46,7 +46,7 @@ public ViewModelPropertyMap(MemberInfo propertyInfo, string name, ProtectMode vi
public bool TransferFirstRequest { get; set; }
/// <summary> When true, an existing object in this property will be preserved during deserialization. A new object will only be created if the property is null, or if we need to call the constructor to set some properties. </summary>
public bool Populate { get; set; }
/// <summary> If true, DotVVM serializer will use JSON converter for the runtime type, instead of resolving one statically </summary>
/// <summary> If true, DotVVM serializer will use JSON converter for the runtime type, instead of resolving one statically. Affects mostly serialization, but also deserialization into an existing instance. </summary>
public bool AllowDynamicDispatch { get; set; }

/// <summary> List of validation rules (~= validation attributes) on this property. Includes rules which can't be run client-side </summary>
Expand Down
Expand Up @@ -62,13 +62,13 @@ internal ViewModelSerializationMap(Type type, IEnumerable<ViewModelPropertyMap>

private void ValidatePropertyMap()
{
var hashset = new HashSet<string>();
var dict = new Dictionary<string, ViewModelPropertyMap>();
foreach (var propertyMap in Properties)
{
if (!hashset.Add(propertyMap.Name))
if (!dict.TryAdd(propertyMap.Name, propertyMap))
{
throw new InvalidOperationException($"Detected member shadowing on property \"{propertyMap.Name}\" " +
$"while building serialization map for \"{Type.ToCode()}\"");
var other = dict[propertyMap.Name];
throw new InvalidOperationException($"Serialization map for '{Type.ToCode()}' has a name conflict between a {(propertyMap.PropertyInfo is FieldInfo ? "field" : "property")} '{propertyMap.PropertyInfo.Name}' and {(other.PropertyInfo is FieldInfo ? "field" : "property")} '{other.PropertyInfo.Name}' — both are named '{propertyMap.Name}' in JSON.");
}
}
}
Expand Down Expand Up @@ -673,7 +673,7 @@ private Expression DeserializePropertyValue(ViewModelPropertyMap property, Expre
if (this.viewModelJsonConverter.CanConvert(type))
{
var defaultConverter = this.viewModelJsonConverter.CreateConverter(type);
if (property.AllowDynamicDispatch)
if (property.AllowDynamicDispatch && !type.IsSealed)
{
return Call(
JsonSerializationCodegenFragments.DeserializeViewModelDynamicMethod.MakeGenericMethod(type),
Expand Down Expand Up @@ -728,7 +728,7 @@ private Expression GetSerializeExpression(ViewModelPropertyMap property, Express
}
if (this.viewModelJsonConverter.CanConvert(value.Type))
{
if (property.AllowDynamicDispatch)
if (property.AllowDynamicDispatch && !value.Type.IsSealed)
{
// TODO: ??
// return Call(
Expand All @@ -745,7 +745,7 @@ private Expression GetSerializeExpression(ViewModelPropertyMap property, Express
}
}

return Call(JsonSerializationCodegenFragments.SerializeValueMethod.MakeGenericMethod(value.Type), writer, jsonOptions, value, Constant(property.AllowDynamicDispatch));
return Call(JsonSerializationCodegenFragments.SerializeValueMethod.MakeGenericMethod(value.Type), writer, jsonOptions, value, Constant(property.AllowDynamicDispatch && !value.Type.IsSealed));
}
}

Expand Down
Expand Up @@ -11,6 +11,7 @@
using System.Text.Json.Serialization;
using System.Text.Json;
using DotVVM.Framework.Compilation.Javascript;
using FastExpressionCompiler;

namespace DotVVM.Framework.ViewModel.Serialization
{
Expand Down Expand Up @@ -117,6 +118,40 @@ protected virtual ViewModelSerializationMap<T> CreateMap<T>()
static Type unwrapByRef(Type t) => t.IsByRef ? t.GetElementType()! : t;
}

protected virtual MemberInfo[] ResolveShadowing(Type type, MemberInfo[] members)
{
var shadowed = new Dictionary<string, MemberInfo>();
foreach (var member in members)
{
if (shadowed.TryAdd(member.Name, member))
continue;
var previous = shadowed[member.Name];
if (member.DeclaringType == previous.DeclaringType)
throw new InvalidOperationException($"Two or more members named '{member.Name}' on type '{member.DeclaringType!.ToCode()}' are not allowed.");
var (inherited, replacing) = member.DeclaringType!.IsAssignableFrom(previous.DeclaringType!) ? (member, previous) : (previous, member);
var inheritedType = inherited.GetResultType();
var replacingType = replacing.GetResultType();

// collections are special case, since everything is serialized as array, we don't have to care about the actual type, only the element type
// this is neccessary for IGridViewDataSet hierarchy to work...
while (ReflectionUtils.IsCollection(inheritedType) && ReflectionUtils.IsCollection(replacingType))
{
inheritedType = ReflectionUtils.GetEnumerableType(inheritedType) ?? typeof(object);
replacingType = ReflectionUtils.GetEnumerableType(replacingType) ?? typeof(object);
}

if (inheritedType.IsAssignableFrom(replacingType))
{
shadowed[member.Name] = replacing;
}
else
{
throw new InvalidOperationException($"Detected forbidden member shadowing of '{inherited.DeclaringType.ToCode(stripNamespace: true)}.{inherited.Name}: {inherited.GetResultType().ToCode(stripNamespace: true)}' by '{replacing.DeclaringType.ToCode(stripNamespace: true)}.{replacing.Name}: {replacing.GetResultType().ToCode(stripNamespace: true)}' while building serialization map for '{type.ToCode(stripNamespace: true)}'");
}
}
return shadowed.Values.ToArray();
}

/// <summary>
/// Gets the properties of the specified type.
/// </summary>
Expand All @@ -127,6 +162,7 @@ protected virtual IEnumerable<ViewModelPropertyMap> GetProperties(Type type, Met
var properties = type.GetAllMembers(BindingFlags.Public | BindingFlags.Instance)
.Where(m => m is PropertyInfo or FieldInfo)
.ToArray();
properties = ResolveShadowing(type, properties);
Array.Sort(properties, (a, b) => StringComparer.Ordinal.Compare(a.Name, b.Name));
foreach (MemberInfo property in properties)
{
Expand All @@ -138,7 +174,7 @@ protected virtual IEnumerable<ViewModelPropertyMap> GetProperties(Type type, Met
include = include ||
!(bindAttribute is null or { Direction: Direction.None }) ||
property.IsDefined(typeof(JsonIncludeAttribute)) ||
(property.DeclaringType.IsGenericType && property.DeclaringType.FullName.StartsWith("System.ValueTuple`"));
(type.IsGenericType && type.FullName.StartsWith("System.ValueTuple`"));
}
if (!include) continue;

Expand Down
89 changes: 88 additions & 1 deletion src/Tests/ViewModel/SerializerTests.cs
Expand Up @@ -409,7 +409,7 @@ public void SupportsSignedDictionary()
CollectionAssert.Contains(obj2.SignedDictionary, new KeyValuePair<string, string>("a", "x"));
CollectionAssert.Contains(obj2.SignedDictionary, new KeyValuePair<string, string>("b", "y"));
Assert.AreEqual(obj.SignedDictionary.Count, obj2.SignedDictionary.Count);
Assert.IsNotNull(json["SignedDictionary"]);
XAssert.IsType<JsonArray>(json["SignedDictionary"]);
}

[TestMethod]
Expand Down Expand Up @@ -578,6 +578,52 @@ public class ViewModelWithUnmatchedConstuctorProperty2
public ViewModelWithUnmatchedConstuctorProperty2(TestViewModelWithByteArray x) { }
}


[TestMethod]
public void PropertyShadowing()
{
var obj = new TestViewModelWithPropertyShadowing.Inner {
EnumerableToList = ["x", "y"],
ObjectToList = ["z" ],
InterfaceToInteger = 5,
ObjectToInteger = 6,
ShadowedByField = 7
};

var (obj2, json) = SerializeAndDeserialize(obj);
XAssert.Equal(obj.EnumerableToList, obj2.EnumerableToList);
XAssert.Equal(obj.ObjectToList, obj2.ObjectToList);
XAssert.Equal(obj.InterfaceToInteger, obj2.InterfaceToInteger);
XAssert.Equal(obj.ObjectToInteger, obj2.ObjectToInteger);
XAssert.Equal(obj.ShadowedByField, obj2.ShadowedByField);
XAssert.IsType<JsonArray>(json["EnumerableToList"]);
}
[TestMethod]
public void PropertyShadowing_BaseTypeDeserialized()
{
var obj = new TestViewModelWithPropertyShadowing.Inner {
EnumerableToList = ["x", "y"],
ObjectToList = ["z" ],
InterfaceToInteger = 5,
ObjectToInteger = 6,
ShadowedByField = 7
};
// Serialized Inner but deserializes the base type
var (obj2Box, json) = SerializeAndDeserialize<DynamicDispatchVMContainer<TestViewModelWithPropertyShadowing>>(new() { Value = obj });
var obj2 = obj2Box.Value;
json = json["Value"].AsObject();
XAssert.Equal(typeof(TestViewModelWithPropertyShadowing), obj2.GetType());

XAssert.Equal(obj.EnumerableToList, obj2.EnumerableToList);
XAssert.IsType<JsonElement>(obj2.ObjectToList);
XAssert.Null(obj2.InterfaceToInteger);
XAssert.Equal(6d, XAssert.IsType<double>(obj2.ObjectToInteger));
XAssert.Equal(7d, XAssert.IsType<double>(obj2.ShadowedByField));
XAssert.Equal(5, (int)json["InterfaceToInteger"]);
XAssert.Equal(6, (int)json["ObjectToInteger"]);
XAssert.Equal(7, (double)json["ShadowedByField"]);
XAssert.IsType<JsonArray>(json["EnumerableToList"]);
}
}

public class DataNode
Expand Down Expand Up @@ -767,4 +813,45 @@ public class TestViewModelWithDateTimes
public DateOnly DateOnly { get; set; }
public TimeOnly TimeOnly { get; set; }
}

public class TestViewModelWithPropertyShadowing
{
public object ObjectToInteger { get; set; }
[JsonIgnore] // does not "inherit" to shadowed property
public IComparable InterfaceToInteger { get; set; }

public IEnumerable<string> EnumerableToList { get; set; }
public object ObjectToList { get; set; }

public object ShadowedByField { get; set; }

public class Inner: TestViewModelWithPropertyShadowing
{
public new int ObjectToInteger { get; set; } = 123;
public new int InterfaceToInteger { get; set; } = 1234;

public new List<string> EnumerableToList { get; set; } = [ "A", "B" ];
public new List<string> ObjectToList { get; set; } = [ "C", "D" ];

public new double ShadowedByField { get; set; } = 12345;
}
}

class DynamicDispatchVMContainer<TStatic>
{
[Bind(AllowDynamicDispatch = true)]
public TStatic Value { get; set; }
}

class StaticDispatchVMContainer<TStatic>
{
[Bind(AllowDynamicDispatch = false)]
public TStatic Value { get; set; }
}

class DefaultDispatchVMContainer<TStatic>
{
[Bind(AllowDynamicDispatch = false)]
public TStatic Value { get; set; }
}
}

0 comments on commit 00e8a55

Please sign in to comment.