Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serializer: only inject properties to constructors of records #1525

Merged
merged 14 commits into from Dec 10, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Framework/Framework/Utils/ReflectionUtils.cs
Expand Up @@ -569,5 +569,9 @@ internal static void ClearCaches(Type[] types)
cache_GetTypeHash.TryRemove(t, out _);
}
}

internal static bool IsInitOnly(this PropertyInfo prop) =>
prop.SetMethod is { ReturnParameter: {} returnParameter } &&
returnParameter.GetRequiredCustomModifiers().Any(t => t == typeof(System.Runtime.CompilerServices.IsExternalInit));
}
}
Expand Up @@ -70,7 +70,7 @@ public void ResetFunctions()
/// <summary>
/// Creates the constructor for this object.
/// </summary>
private Expression CallConstructor(Expression services, Dictionary<PropertyInfo, ParameterExpression> propertyVariables)
private Expression CallConstructor(Expression services, Dictionary<PropertyInfo, ParameterExpression> propertyVariables, bool throwImmediately = false)
{
if (constructorFactory != null)
return Convert(Invoke(Constant(constructorFactory), services), Type);
Expand All @@ -82,10 +82,10 @@ private Expression CallConstructor(Expression services, Dictionary<PropertyInfo,
}

if (Constructor is null && (Type.IsInterface || Type.IsAbstract))
throw new Exception($"Can not deserialize {Type.ToCode()} because it's abstract. Please avoid using abstract types in view model. If you really mean it, you can add a static factory method and mark it with [JsonConstructor] attribute.");
return jitException($"Can not deserialize {Type.ToCode()} because it's abstract. Please avoid using abstract types in view model. If you really mean it, you can add a static factory method and mark it with [JsonConstructor] attribute.");

if (Constructor is null)
throw new Exception($"Can not deserialize {Type.ToCode()}, no constructor or multiple constructors found. Use the [JsonConstructor] attribute to specify the constructor used for deserialization.");
return jitException($"Can not deserialize {Type.ToCode()}, no parameterless constructor found. Use the [JsonConstructor] attribute to specify the constructor used for deserialization.");

var parameters = Constructor.GetParameters().Select(p => {
var prop = Properties.FirstOrDefault(pp => pp.ConstructorParameter == p);
Expand Down Expand Up @@ -116,6 +116,16 @@ private Expression CallConstructor(Expression services, Dictionary<PropertyInfo,
Call(m, parameters),
_ => throw new NotSupportedException()
};

// Since the old serializer didn't care about constructor problems until it was actually needed,
// we can't throw exception during compilation, so we wait until this code will run.
Expression jitException(string message) =>
throwImmediately
? throw new Exception(message)
: Expression.Throw(Expression.New(
typeof(Exception).GetConstructor(new [] { typeof(string) })!,
Expression.Constant(message)
), this.Type);
}

/// <summary>
Expand All @@ -141,14 +151,28 @@ public ReaderDelegate CreateReaderFactory()
p => Expression.Variable(p.Type, "prop_" + p.Name)
);

var hasConstructorProperties = Properties.Any(p => p.ConstructorParameter is {});
var constructorCall = CallConstructor(servicesParameter, propertyVars);
// If we have constructor property or if we have { get; init; } property, we always create new instance
var alwaysCallConstructor = Properties.Any(p => p.TransferToServer && (
p.ConstructorParameter is {} ||
p.PropertyInfo.IsInitOnly()));

// We don't want to clone IDotvvmViewModel automatically, because the user is likely to register this specific instance somewhere
if (alwaysCallConstructor && typeof(IDotvvmViewModel).IsAssignableFrom(Type) && Constructor is {} && Constructor.IsDefined(typeof(JsonConstructorAttribute)))
{
var cloneReason =
Properties.FirstOrDefault(p => p.TransferToServer && p.PropertyInfo.IsInitOnly()) is {} initProperty
? $"init-only property {initProperty.Name} is transferred client → server" :
Properties.FirstOrDefault(p => p.TransferToServer && p.ConstructorParameter is {}) is {} ctorProperty
? $"property {ctorProperty.Name} must be injected into constructor parameter {ctorProperty.ConstructorParameter!.Name}" : "internal bug";
throw new Exception($"Deserialization of {Type.ToCode()} is not allowed, because it implements IDotvvmViewModel and {cloneReason}. To allow cloning the object on deserialization, mark a constructor with [JsonConstructor].");
}
var constructorCall = CallConstructor(servicesParameter, propertyVars, throwImmediately: alwaysCallConstructor);

// curly brackets are used for variables and methods from the context of this factory method
// value = ({Type})valueParam;
block.Add(Assign(value,
Condition(Equal(valueParam, Constant(null)),
hasConstructorProperties
alwaysCallConstructor
? Default(Type)
: constructorCall,
Convert(valueParam, Type)
Expand All @@ -170,7 +194,6 @@ public ReaderDelegate CreateReaderFactory()
// add current object to encrypted values, this is needed because one property can potentially contain more objects (is a collection)
block.Add(Expression.Call(encryptedValuesReader, nameof(EncryptedValuesReader.Nest), Type.EmptyTypes));


// if the reader is in an invalid state, throw an exception
// TODO: Change exception type, just Exception is not exactly descriptive
block.Add(ExpressionUtils.Replace((JsonReader rdr) => rdr.TokenType == JsonToken.StartObject ? rdr.Read() : ExpressionUtils.Stub.Throw<bool>(new Exception($"TokenType = StartObject was expected.")), reader));
Expand Down Expand Up @@ -323,7 +346,7 @@ public ReaderDelegate CreateReaderFactory()
block.Add(Expression.Call(encryptedValuesReader, nameof(EncryptedValuesReader.AssertEnd), Type.EmptyTypes));

// call the constructor
if (hasConstructorProperties)
if (alwaysCallConstructor)
{
block.Add(Assign(value, constructorCall));
}
Expand Down
Expand Up @@ -41,26 +41,60 @@ public class ViewModelSerializationMapper : IViewModelSerializationMapper
/// </summary>
protected virtual ViewModelSerializationMap CreateMap(Type type)
{
var constructor = GetConstructor(type);
return new ViewModelSerializationMap(type, GetProperties(type, constructor), constructor, configuration);
// constructor which takes properties as parameters
// if it exists, we always need to recreate the viewmodel
var valueConstructor = GetConstructor(type);
return new ViewModelSerializationMap(type, GetProperties(type, valueConstructor), valueConstructor, configuration);
}

protected virtual MethodBase? GetConstructor(Type type)
{
if (ReflectionUtils.IsPrimitiveType(type) || ReflectionUtils.IsEnumerable(type))
return null;

if (type.GetConstructors().FirstOrDefault(c => c.IsDefined(typeof(JsonConstructorAttribute))) is {} ctor)
return ctor;
if (type.GetMethods(BindingFlags.Static | BindingFlags.Public).FirstOrDefault(c => c.IsDefined(typeof(JsonConstructorAttribute))) is {} factory)
return factory;

if (type.IsAbstract)
return null;

if (type.GetConstructors().FirstOrDefault(c => c.IsDefined(typeof(JsonConstructorAttribute))) is {} ctor)
return ctor;

if (type.GetConstructor(Type.EmptyTypes) is {} emptyCtor)
return emptyCtor;
var constructors = type.GetConstructors();
return GetRecordConstructor(type);
}

protected static MethodBase? GetRecordConstructor(Type t)
{
if (t.IsAbstract)
return null;
// https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEAzAzmgFxCgFcA7AHwgAcYyACAZQE9cCYBbAWACga6mrdhwB0AGQCWZAI68CzWvQDC9ALz0A3vQBCIelIIBuZXoPGwpsgXoBfOQpj0AqmvoANehXoBNehGz6Vry81FAG2AwARACCkcE8GDDW1urytP4APEoAfPGJ1gCGBARQrgQiAOJJSiRsEBzRxWHAJOy4ACJFBQAUAJQi0WTM3djk9AX0cNnjA00SLewAKg4iAGIkGBgAcgUcjuqRALISYFAQuP7lq4wAFgVQ1CJK0DBP9dQSGEUSEGSHBdQPmQAOaNErzVowSL0ABkMOUvwAbjAoOVFhAAJJWADMACZugU3mQ2KQwARoNEoMCSHsrLgANoABgAuiIAGoFDAkGC9Vy43ohMJWCL0SJFEp6ACksXGTSAA=
// F# record or single case discriminated union (multi case is abstract)
if (t.GetCustomAttributesData().Any(a => a.AttributeType.FullName == "Microsoft.FSharp.Core.CompilationMappingAttribute" && Convert.ToInt32(a.ConstructorArguments[0].Value) is 1 or 2))
{
return t.GetConstructors().Single();
}
// TODO: F# normal type, it's not possible AFAIK to add attribute to the default constructor

// find constructor which matches Deconstruct method
var deconstruct = t.GetMethods(BindingFlags.Instance | BindingFlags.Public).Where(m => m.Name == "Deconstruct").ToArray();
if (deconstruct.Length == 0)
return null;
var constructors =
(from c in t.GetConstructors()
from d in deconstruct
where c.GetParameters().Select(p => p.Name).SequenceEqual(d.GetParameters().Select(p => p.Name)) &&
c.GetParameters().Select(p => unwrapByRef(p.ParameterType)).SequenceEqual(d.GetParameters().Select(p => unwrapByRef(p.ParameterType)))
select c).ToArray();

if (constructors.Length == 1)
return constructors[0];

return null;

static Type unwrapByRef(Type t) => t.IsByRef ? t.GetElementType()! : t;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Samples/Tests/Tests/Control/ComboBoxTests.cs
Expand Up @@ -230,7 +230,7 @@ public void Control_ComboBox_ItemBinding_ItemValueBinding_SelectedValue_StringTo
RunInAllBrowsers(browser => {
browser.NavigateToUrl(SamplesRouteUrls.ControlSamples_ComboBox_ItemBinding_ItemValueBinding_SelectedValue_StringToInt_Error);

AssertUI.InnerText(browser.First(".exceptionMessage"), s => s.Contains("System.String") && s.Contains("not assignable") && s.Contains("System.Int32"));
AssertUI.InnerText(browser.First(".exceptionMessage"), s => s.Contains("string") && s.Contains("not assignable") && s.Contains("System.Int32"));
AssertUI.InnerText(browser.First("p.summary"), s => s.Contains("DotVVM.Framework.Compilation.DotvvmCompilationException"));
AssertUI.InnerText(browser.First(".errorUnderline"), s => s.Contains("{value: SelectedInt}"));
});
Expand Down
31 changes: 30 additions & 1 deletion src/Tests/ControlTests/CommandTests.cs
Expand Up @@ -4,6 +4,7 @@
using System.Threading.Tasks;
using Newtonsoft.Json;
using Microsoft.Extensions.DependencyInjection;
using DotVVM.Framework.ViewModel;

namespace DotVVM.Framework.Tests.ControlTests
{
Expand Down Expand Up @@ -32,7 +33,7 @@ public async Task RootViewModelIsRecord()
}


public class ViewModel1
public class ViewModel1: DotvvmViewModelBase
{
// don't ask me why people do this...
// on one project, 4.1 upgrade did not work, because they try to inject
Expand All @@ -44,6 +45,34 @@ public ViewModel1(OmgViewModelWithIsAlsoAService nestedVM)
NestedVM = nestedVM;
}

private bool calledInit, calledLoad;

public override Task Init()
{
if (Context is null)
throw new System.Exception("Context is null in Init");
calledInit = true;
return base.Init();
}
public override Task Load()
{
if (Context is null)
throw new System.Exception("Context is null in Load");
if (!calledInit)
throw new System.Exception("Init was not called");
calledLoad = true;
return base.Load();
}
public override Task PreRender()
{
if (Context is null)
throw new System.Exception("Context is null in PreRender");
if (!calledLoad)
throw new System.Exception("Load was not called");
return base.PreRender();
}


public OmgViewModelWithIsAlsoAService NestedVM { get; }

public string Text { get; set; } = "Text1";
Expand Down