diff --git a/src/Framework/Core/ViewModel/BindAttribute.cs b/src/Framework/Core/ViewModel/BindAttribute.cs index 01797114b7..112da2bdf2 100644 --- a/src/Framework/Core/ViewModel/BindAttribute.cs +++ b/src/Framework/Core/ViewModel/BindAttribute.cs @@ -7,7 +7,7 @@ namespace DotVVM.Framework.ViewModel /// /// Specifies the binding direction. /// - [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)] + [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)] public class BindAttribute : Attribute { @@ -24,8 +24,8 @@ public class BindAttribute : Attribute public bool? _allowDynamicDispatch; /// /// 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). /// public bool AllowDynamicDispatch { get => _allowDynamicDispatch ?? false; set => _allowDynamicDispatch = value; } diff --git a/src/Framework/Framework/ViewModel/Serialization/ViewModelJsonConverter.cs b/src/Framework/Framework/ViewModel/Serialization/ViewModelJsonConverter.cs index 2372c999fa..dabbf5e1d6 100644 --- a/src/Framework/Framework/ViewModel/Serialization/ViewModelJsonConverter.cs +++ b/src/Framework/Framework/ViewModel/Serialization/ViewModelJsonConverter.cs @@ -3,8 +3,6 @@ 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; @@ -12,9 +10,7 @@ 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 { @@ -69,6 +65,8 @@ public class VMConverter(ViewModelJsonConverter factory): JsonConverter, 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) { @@ -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) => diff --git a/src/Framework/Framework/ViewModel/Serialization/ViewModelPropertyMap.cs b/src/Framework/Framework/ViewModel/Serialization/ViewModelPropertyMap.cs index fc191a9af9..53f38fa145 100644 --- a/src/Framework/Framework/ViewModel/Serialization/ViewModelPropertyMap.cs +++ b/src/Framework/Framework/ViewModel/Serialization/ViewModelPropertyMap.cs @@ -46,7 +46,7 @@ public ViewModelPropertyMap(MemberInfo propertyInfo, string name, ProtectMode vi public bool TransferFirstRequest { get; set; } /// 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. public bool Populate { get; set; } - /// If true, DotVVM serializer will use JSON converter for the runtime type, instead of resolving one statically + /// 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. public bool AllowDynamicDispatch { get; set; } /// List of validation rules (~= validation attributes) on this property. Includes rules which can't be run client-side diff --git a/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMap.cs b/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMap.cs index b072977559..d585be850d 100644 --- a/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMap.cs +++ b/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMap.cs @@ -62,13 +62,13 @@ public static ViewModelSerializationMap Create(Type type, IEnumerable(); + var dict = new Dictionary(); 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."); } } } @@ -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), @@ -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( @@ -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)); } } diff --git a/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMapper.cs b/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMapper.cs index 57d868a913..cfc5a8cce7 100644 --- a/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMapper.cs +++ b/src/Framework/Framework/ViewModel/Serialization/ViewModelSerializationMapper.cs @@ -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 { @@ -117,6 +118,40 @@ where c.GetParameters().Select(p => p.Name).SequenceEqual(d.GetParameters().Sele static Type unwrapByRef(Type t) => t.IsByRef ? t.GetElementType()! : t; } + protected virtual MemberInfo[] ResolveShadowing(Type type, MemberInfo[] members) + { + var shadowed = new Dictionary(); + 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(); + } + /// /// Gets the properties of the specified type. /// @@ -127,6 +162,7 @@ protected virtual IEnumerable 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) { @@ -138,7 +174,7 @@ protected virtual IEnumerable 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; diff --git a/src/Tests/ViewModel/SerializerTests.cs b/src/Tests/ViewModel/SerializerTests.cs index 54c491c704..67b57fe27f 100644 --- a/src/Tests/ViewModel/SerializerTests.cs +++ b/src/Tests/ViewModel/SerializerTests.cs @@ -409,7 +409,7 @@ public void SupportsSignedDictionary() CollectionAssert.Contains(obj2.SignedDictionary, new KeyValuePair("a", "x")); CollectionAssert.Contains(obj2.SignedDictionary, new KeyValuePair("b", "y")); Assert.AreEqual(obj.SignedDictionary.Count, obj2.SignedDictionary.Count); - Assert.IsNotNull(json["SignedDictionary"]); + XAssert.IsType(json["SignedDictionary"]); } [TestMethod] @@ -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(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>(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(obj2.ObjectToList); + XAssert.Null(obj2.InterfaceToInteger); + XAssert.Equal(6d, XAssert.IsType(obj2.ObjectToInteger)); + XAssert.Equal(7d, XAssert.IsType(obj2.ShadowedByField)); + XAssert.Equal(5, (int)json["InterfaceToInteger"]); + XAssert.Equal(6, (int)json["ObjectToInteger"]); + XAssert.Equal(7, (double)json["ShadowedByField"]); + XAssert.IsType(json["EnumerableToList"]); + } } public class DataNode @@ -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 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 EnumerableToList { get; set; } = [ "A", "B" ]; + public new List ObjectToList { get; set; } = [ "C", "D" ]; + + public new double ShadowedByField { get; set; } = 12345; + } + } + + class DynamicDispatchVMContainer + { + [Bind(AllowDynamicDispatch = true)] + public TStatic Value { get; set; } + } + + class StaticDispatchVMContainer + { + [Bind(AllowDynamicDispatch = false)] + public TStatic Value { get; set; } + } + + class DefaultDispatchVMContainer + { + [Bind(AllowDynamicDispatch = false)] + public TStatic Value { get; set; } + } } diff --git a/src/Tests/ViewModel/ViewModelSerializationMapperTests.cs b/src/Tests/ViewModel/ViewModelSerializationMapperTests.cs index b477ef73db..d67d2d9f30 100644 --- a/src/Tests/ViewModel/ViewModelSerializationMapperTests.cs +++ b/src/Tests/ViewModel/ViewModelSerializationMapperTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Text; using System.Text.Json.Serialization; using DotVVM.Framework.Configuration; @@ -10,6 +11,7 @@ using FastExpressionCompiler; using Microsoft.Extensions.DependencyInjection; using Microsoft.VisualStudio.TestTools.UnitTesting; +using NJ=Newtonsoft.Json; namespace DotVVM.Framework.Tests.ViewModel { @@ -33,25 +35,74 @@ public void ViewModelSerializationMapper_Name_JsonPropertyVsBindAttribute() } [TestMethod] - public void ViewModelSerializationMapper_Name_MemberShadowing() + public void ViewModelSerializationMapper_Name_NewtonsoftJsonAttributes() { + // we still respect NJ attributes var mapper = DotvvmTestHelper.DefaultConfig.ServiceProvider.GetRequiredService(); + var map = mapper.GetMap(typeof(NewtonsoftJsonAttributes)); - var exception = XAssert.ThrowsAny(() => mapper.GetMap(typeof(MemberShadowingViewModelB))); + XAssert.DoesNotContain("Ignored", map.Properties.Select(p => p.Name)); + Assert.AreEqual("new_name", map.Property("RenamedProperty").Name); + } + + [DataTestMethod] + [DataRow(typeof(MemberShadowingViewModelB), "Property1", "List", "List>")] + [DataRow(typeof(MemberShadowingViewModelC), "Property1", "List", "object")] + [DataRow(typeof(MemberShadowingViewModelD), "Property2", "ViewModelSerializationMapperTests.JsonPropertyVsBindAttribute", "object")] + [DataRow(typeof(MemberShadowingViewModelE), "Property2", "ViewModelSerializationMapperTests.JsonPropertyVsBindAttribute", "TestViewModelWithBind")] + public void ViewModelSerializationMapper_Name_MemberShadowing(Type type, string prop, string t1, string t2) + { + var mapper = DotvvmTestHelper.DefaultConfig.ServiceProvider.GetRequiredService(); + + var exception = XAssert.ThrowsAny(() => mapper.GetMap(type)); + XAssert.IsType(exception.GetBaseException()); + XAssert.Equal($"Detected forbidden member shadowing of 'ViewModelSerializationMapperTests.MemberShadowingViewModelA.{prop}: {t1}' by '{type.ToCode(stripNamespace: true)}.{prop}: {t2}' while building serialization map for '{type.ToCode(stripNamespace: true)}'", exception.GetBaseException().Message); + } + + [TestMethod] + public void ViewModelSerializationMapper_Name_NameConflictAttributes() + { + var mapper = DotvvmTestHelper.DefaultConfig.ServiceProvider.GetRequiredService(); + var exception = XAssert.ThrowsAny(() => mapper.GetMap(typeof(NameConflictAttributes))); + XAssert.IsType(exception.GetBaseException()); + Assert.AreEqual("Serialization map for 'DotVVM.Framework.Tests.ViewModel.ViewModelSerializationMapperTests.NameConflictAttributes' has a name conflict between a property 'Name' and property 'MyProperty' — both are named 'Name' in JSON.", exception.GetBaseException().Message); + } + + [TestMethod] + public void ViewModelSerializationMapper_Name_NameConflictFieldProperty() + { + var mapper = DotvvmTestHelper.DefaultConfig.ServiceProvider.GetRequiredService(); + var exception = XAssert.ThrowsAny(() => mapper.GetMap(typeof(NameConflictFieldProperty))); XAssert.IsType(exception.GetBaseException()); - XAssert.Equal($"Detected member shadowing on property \"{nameof(MemberShadowingViewModelB.Property)}\" while building serialization map for \"{typeof(MemberShadowingViewModelB).ToCode()}\"", exception.GetBaseException().Message); + Assert.AreEqual("Serialization map for 'DotVVM.Framework.Tests.ViewModel.ViewModelSerializationMapperTests.NameConflictFieldProperty' has a name conflict between a property 'Name' and field 'MyField' — both are named 'Name' in JSON.", exception.GetBaseException().Message); } public class MemberShadowingViewModelA { - public object Property { get; set; } + public List Property1 { get; set; } + public JsonPropertyVsBindAttribute Property2 { get; set; } } public class MemberShadowingViewModelB : MemberShadowingViewModelA { -#pragma warning disable CS0108 // Member hides inherited member; missing new keyword - public string Property { get; set; } -#pragma warning restore CS0108 // Member hides inherited member; missing new keyword + // different type + public new List> Property1 { get; set; } + } + + public class MemberShadowingViewModelC : MemberShadowingViewModelA + { + // more generic + public new object Property1 { get; set; } + } + public class MemberShadowingViewModelD : MemberShadowingViewModelA + { + // more generic + public new object Property2 { get; set; } + } + public class MemberShadowingViewModelE : MemberShadowingViewModelA + { + // different type + public new TestViewModelWithBind Property2 { get; set; } } public class JsonPropertyVsBindAttribute @@ -77,7 +128,30 @@ public class JsonPropertyVsBindAttribute [Bind()] [JsonPropertyName("jsonProperty3")] public string BindWithoutNameJsonPropertyWithName { get; set; } + } + public class NewtonsoftJsonAttributes + { + [NJ.JsonIgnore] + public bool Ignored { get; set; } + + [NJ.JsonProperty("new_name")] + public string RenamedProperty { get; set; } + } + + public class NameConflictAttributes + { + [Bind(Name = "Name")] + public string MyProperty { get; set; } + + public string Name { get; set; } + } + + public class NameConflictFieldProperty + { + public string Name { get; set; } + [Bind(Name = "Name")] + public string MyField; } }