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

[Serialization] Fix diverging rules for editor and runtime serialization of fields and properties #1875

Merged
merged 11 commits into from Oct 16, 2023
Expand Up @@ -65,7 +65,8 @@ public MyAsset()

public MyDictionaryPure MapItems3 { get; set; }

public object CustomObjectWithProtectedSet { get; protected set; }
[DataMember]
public CustomObject CustomObjectWithProtectedSet { get; protected set; }
}

[DataContract("CustomObject")]
Expand Down
5 changes: 5 additions & 0 deletions sources/assets/Stride.Core.Assets/AssetItem.cs
Expand Up @@ -59,6 +59,7 @@ internal AssetItem([NotNull] UFile location, [NotNull] Asset asset, Package pack
/// </summary>
/// <value>The location.</value>
[NotNull]
[DataMember]
public UFile Location { get => location; internal set => location = value ?? throw new ArgumentNullException(nameof(value)); }

/// <summary>
Expand All @@ -84,6 +85,7 @@ internal AssetItem([NotNull] UFile location, [NotNull] Asset asset, Package pack
/// Gets the package where this asset is stored.
/// </summary>
/// <value>The package.</value>
[DataMember]
public Package Package { get; internal set; }

/// <summary>
Expand Down Expand Up @@ -187,6 +189,7 @@ public UFile FullPath
/// </summary>
/// <value>The asset.</value>
[NotNull]
[DataMember]
public Asset Asset { get => asset; internal set => asset = value ?? throw new ArgumentNullException(nameof(value)); }

/// <summary>
Expand All @@ -197,13 +200,15 @@ public UFile FullPath
/// By default, contains the last modified time of the asset from the disk. If IsDirty is also updated from false to true
/// , this time will get current time of modification.
/// </remarks>
[DataMember]
public DateTime ModifiedTime { get; internal set; }

private long version;

/// <summary>
/// Gets the asset version incremental counter, increased everytime the asset is edited.
/// </summary>
[DataMember]
public long Version { get => Interlocked.Read(ref version); internal set => Interlocked.Exchange(ref version, value); }

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions sources/assets/Stride.Core.Assets/Bundle.cs
Expand Up @@ -33,12 +33,14 @@ public Bundle()
/// Gets the selectors used by this bundle.
/// </summary>
/// <value>The selectors.</value>
[DataMember]
public List<AssetSelector> Selectors { get; private set; }

/// <summary>
/// Gets the bundle dependencies.
/// </summary>
/// <value>The dependencies.</value>
[DataMember]
public List<string> Dependencies { get; private set; }

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions sources/assets/Stride.Core.Assets/Selectors/TagSelector.cs
Expand Up @@ -25,6 +25,7 @@ public TagSelector()
/// Gets the tags that will be used to select an asset.
/// </summary>
/// <value>The tags.</value>
[DataMember]
public TagCollection Tags { get; private set; }

public override IEnumerable<string> Select(PackageSession packageSession, IContentIndexMap contentIndexMap)
Expand Down
2 changes: 2 additions & 0 deletions sources/assets/Stride.Core.Assets/SolutionPlatform.cs
Expand Up @@ -283,6 +283,7 @@ public SolutionConfiguration(string name)
/// Gets or sets the configuration name (e.g. Debug, Release)
/// </summary>
/// <value>The name.</value>
[DataMember]
public string Name { get; private set; }

/// <summary>
Expand All @@ -295,6 +296,7 @@ public SolutionConfiguration(string name)
/// Gets the additional msbuild properties for a specific configuration (Debug or Release)
/// </summary>
/// <value>The msbuild configuration properties.</value>
[DataMember]
public List<string> Properties { get; private set; }
}
}
1 change: 1 addition & 0 deletions sources/assets/Stride.Core.Assets/Yaml/YamlAssetPath.cs
Expand Up @@ -127,6 +127,7 @@ public YamlAssetPath([NotNull] IEnumerable<Element> elements)
/// <summary>
/// The elements constituting this path.
/// </summary>
[DataMember]
public IReadOnlyList<Element> Elements => elements;

/// <summary>
Expand Down
Expand Up @@ -10,7 +10,6 @@
using System.Runtime.CompilerServices;
using System.Text;
using Mono.Cecil;
using Stride.Core;

namespace Stride.Core.AssemblyProcessor
{
Expand Down Expand Up @@ -296,11 +295,14 @@ public static IEnumerable<SerializableItem> GetSerializableItems(TypeDefinition
foreach (var property in type.Properties)
{
// Need a non-static public get method
if (property.GetMethod == null || !property.GetMethod.IsPublic || property.GetMethod.IsStatic)
if (property.GetMethod.IsStatic)
continue;

// If it's a struct (!IsValueType), we need a public set method as well
if (property.PropertyType.IsValueType && (property.SetMethod == null || !(property.SetMethod.IsAssembly || property.SetMethod.IsPublic)))
if (IsAccessibleThroughAccessModifiers(property) == false)
continue;

// If it's a struct (!IsValueType), we need a set method as well
if (property.PropertyType.IsValueType && property.SetMethod == null)
continue;

// Only take virtual properties (override ones will be handled by parent serializers)
Expand Down Expand Up @@ -382,6 +384,27 @@ public static IEnumerable<SerializableItem> GetSerializableItems(TypeDefinition
}
}

static bool IsAccessibleThroughAccessModifiers(PropertyDefinition property)
{
var get = property.GetMethod;
var set = property.SetMethod;

if (get == null)
return false;

bool forced = property.CustomAttributes.Any(a => a.AttributeType.FullName == "Stride.Core.DataMemberAttribute");
Eideren marked this conversation as resolved.
Show resolved Hide resolved

if (forced && (get.IsPublic || get.IsAssembly))
return true;

// By default, allow access for get-only auto-property, i.e.: { get; } but not { get => }
// as the later may create side effects, and without a setter, we can't 'set' as a fallback for those exceptional cases.
if (get.IsPublic)
return set?.IsPublic == true || set == null && property.DeclaringType.Fields.Any(x => x.Name == $"<{property.Name}>k__BackingField");
Eideren marked this conversation as resolved.
Show resolved Hide resolved

return false;
}

internal static bool IsMemberIgnored(ICollection<CustomAttribute> customAttributes, ComplexTypeSerializerFlags flags, DataMemberMode dataMemberMode)
{
// Check for DataMemberIgnore
Expand Down
1 change: 1 addition & 0 deletions sources/core/Stride.Core.Design/Settings/SettingsFile.cs
Expand Up @@ -22,6 +22,7 @@ public SettingsFile(SettingsProfile profile)
/// <summary>
/// Gets the settings profile to serialize.
/// </summary>
[DataMember]
[DataMemberCustomSerializer]
public SettingsProfile Settings { get; private set; }
}
Expand Down
Expand Up @@ -30,7 +30,7 @@ public FieldDescriptor(ITypeDescriptor typeDescriptor, FieldInfo fieldInfo, Stri

public override bool IsPublic => FieldInfo.IsPublic;

public override bool HasSet => true;
public override bool HasSet => !FieldInfo.IsInitOnly;

public override object Get(object thisObject)
{
Expand Down
Expand Up @@ -21,11 +21,9 @@ public PropertyDescriptor(ITypeDescriptor typeDescriptor, PropertyInfo propertyI

PropertyInfo = propertyInfo;

getMethod = propertyInfo.GetGetMethod(false) ?? propertyInfo.GetGetMethod(true);
if (propertyInfo.CanWrite && propertyInfo.GetSetMethod(!IsPublic) != null)
{
setMethod = propertyInfo.GetSetMethod(!IsPublic);
}
getMethod = propertyInfo.GetMethod;
setMethod = propertyInfo.SetMethod;

TypeDescriptor = typeDescriptor;
}

Expand Down
Expand Up @@ -213,19 +213,21 @@ protected virtual List<IMemberDescriptor> PrepareMembers()
}

// TODO: we might want an option to disable non-public.
if (Category == DescriptorCategory.Object || Category == DescriptorCategory.NotSupportedObject)
if (Category is DescriptorCategory.Object or DescriptorCategory.NotSupportedObject)
bindingFlags |= BindingFlags.NonPublic;

var memberList = (from propertyInfo in Type.GetProperties(bindingFlags)
where propertyInfo.CanRead && propertyInfo.GetIndexParameters().Length == 0 && IsMemberToVisit(propertyInfo)
where Type.IsAnonymous() || IsAccessibleThroughAccessModifiers(propertyInfo)
where propertyInfo.GetIndexParameters().Length == 0 && IsMemberToVisit(propertyInfo)
select new PropertyDescriptor(factory.Find(propertyInfo.PropertyType), propertyInfo, NamingConvention.Comparer)
into member
where PrepareMember(member, metadataClassMemberInfos?.FirstOrDefault(x => x.MemberInfo.Name == member.OriginalName && x.MemberType == member.Type).MemberInfo)
select member as IMemberDescriptor).ToList();

// Add all public fields
memberList.AddRange(from fieldInfo in Type.GetFields(bindingFlags)
where fieldInfo.IsPublic && IsMemberToVisit(fieldInfo)
where (fieldInfo.IsPublic || (fieldInfo.IsAssembly && fieldInfo.GetCustomAttribute<DataMemberAttribute>() != null))
where IsMemberToVisit(fieldInfo)
select new FieldDescriptor(factory.Find(fieldInfo.FieldType), fieldInfo, NamingConvention.Comparer)
into member
where PrepareMember(member, metadataClassMemberInfos?.FirstOrDefault(x => x.MemberInfo.Name == member.OriginalName && x.MemberType == member.Type).MemberInfo)
Expand All @@ -237,6 +239,33 @@ where PrepareMember(member, metadataClassMemberInfos?.FirstOrDefault(x => x.Memb
return memberList;
}

static bool IsAccessibleThroughAccessModifiers(PropertyInfo property)
{
var get = property.GetMethod;
var set = property.SetMethod;

if (get == null)
return false;

bool forced = property.GetCustomAttribute<DataMemberAttribute>() is not null;

if (forced && (get.IsPublic || get.IsAssembly))
return true;

// By default, allow access for get-only auto-property, i.e.: { get; } but not { get => }
// as the later may create side effects, and without a setter, we can't 'set' as a fallback for those exceptional cases.
if (get.IsPublic)
return set?.IsPublic == true || set == null && TryGetBackingField(property, out _);
Eideren marked this conversation as resolved.
Show resolved Hide resolved

return false;
}

static bool TryGetBackingField(PropertyInfo property, out FieldInfo backingField)
{
backingField = property.DeclaringType.GetField($"<{property.Name}>k__BackingField", BindingFlags.Instance | BindingFlags.NonPublic);
return backingField != null;
}

protected virtual bool PrepareMember(MemberDescriptorBase member, MemberInfo metadataClassMemberInfo)
{
var memberType = member.Type;
Expand All @@ -253,8 +282,7 @@ protected virtual bool PrepareMember(MemberDescriptorBase member, MemberInfo met
}

// Gets the style
var styleAttribute = attributes.FirstOrDefault(x => x is DataStyleAttribute) as DataStyleAttribute;
if (styleAttribute != null)
if (attributes.FirstOrDefault(x => x is DataStyleAttribute) is DataStyleAttribute styleAttribute)
{
member.Style = styleAttribute.Style;
member.ScalarStyle = styleAttribute.ScalarStyle;
Expand All @@ -265,14 +293,12 @@ protected virtual bool PrepareMember(MemberDescriptorBase member, MemberInfo met
if (memberAttribute != null)
{
((IMemberDescriptor)member).Mask = memberAttribute.Mask;
member.Mode = memberAttribute.Mode;
if (!member.HasSet)
{
if (memberAttribute.Mode == DataMemberMode.Assign ||
(memberType.IsValueType && member.Mode == DataMemberMode.Content))
throw new ArgumentException($"{memberType.FullName} {member.OriginalName} is not writeable by {memberAttribute.Mode.ToString()}.");
if (memberAttribute.Mode == DataMemberMode.Assign || memberType.IsValueType || memberType == typeof(string))
member.Mode = DataMemberMode.Never;
}

member.Mode = memberAttribute.Mode;
member.Order = memberAttribute.Order;
}

Expand All @@ -292,16 +318,14 @@ protected virtual bool PrepareMember(MemberDescriptorBase member, MemberInfo met
DefaultValueAttribute defaultValueAttribute = null;
foreach (var attribute in attributes)
{
var valueAttribute = attribute as DefaultValueAttribute;
if (valueAttribute != null)
if (attribute is DefaultValueAttribute valueAttribute)
{
// If we've already found one, don't overwrite it
defaultValueAttribute = defaultValueAttribute ?? valueAttribute;
continue;
}

var yamlRemap = attribute as DataAliasAttribute;
if (yamlRemap != null)
if (attribute is DataAliasAttribute yamlRemap)
{
if (member.AlternativeNames == null)
{
Expand Down
2 changes: 1 addition & 1 deletion sources/core/Stride.Core.Yaml.Tests/DescriptorTests.cs
Expand Up @@ -77,7 +77,7 @@ public TestObject()

public ICollection<string> Collection { get; set; }

public ICollection<string> CollectionReadOnly { get; private set; }
public ICollection<string> CollectionReadOnly { get; }

[DataMemberIgnore]
public string DontSerialize { get; set; }
Expand Down
Expand Up @@ -221,6 +221,7 @@ public MyObject()

public int[] Array { get; set; }

[DataMember]
public int[] ArrayContent { get; private set; }
}

Expand Down Expand Up @@ -576,6 +577,7 @@ public MyCustomClassWithSpecialMembers()
/// value of the list stored in this instance instead of
/// creating a new List&lt;T&gtl instance.
/// </summary>
[DataMember]
public List<string> StringListByContent { get; private set; }

/// <summary>
Expand All @@ -595,6 +597,7 @@ public MyCustomClassWithSpecialMembers()
/// Idem as for <see cref="StringListByContent"/> but for dictionary.
/// </summary>
/// <value>The content of the string mapby.</value>
[DataMember]
public Dictionary<string, object> StringMapbyContent { get; private set; }

/// <summary>
Expand All @@ -603,7 +606,8 @@ public MyCustomClassWithSpecialMembers()
/// creating a new List&lt;T&gtl instance.
/// </summary>
/// <value>The content of the list by.</value>
public IList ListByContent { get; private set; }
[DataMember]
public List<string> ListByContent { get; private set; }
}

/// <summary>
Expand Down Expand Up @@ -1506,21 +1510,6 @@ public class ObjectWithMask
internal int Int3 { get; set; }
}

[Fact]
public void TestImplicitMemberType()
{
var settings = new SerializerSettings() {LimitPrimitiveFlowSequence = 0};

var text = @"!ClassWithImplicitMemberType
Test:
String: test
";

settings.RegisterTagMapping("ClassWithImplicitMemberType", typeof(ClassWithImplicitMemberType));
settings.RegisterTagMapping("ClassWithImplicitMemberTypeInner", typeof(ClassWithImplicitMemberTypeInner));
SerialRoundTrip(settings, text);
}

Comment on lines -1509 to -1523
Copy link
Collaborator Author

@Eideren Eideren Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has been removed as omitting object type when they match the type at initialization but diverge from the member's return type has been deprecated in a previous pr.
You'll see more changes related to this in other tests.

[Fact]
public void TestNonImplicitMemberType()
{
Expand Down Expand Up @@ -1567,6 +1556,7 @@ public ClassWithImplicitMemberType()
Test = new ClassWithImplicitMemberTypeInner {String = "test"};
}

[DataMember]
public object Test { get; protected set; }
}

Expand Down
6 changes: 3 additions & 3 deletions sources/core/Stride.Core/DataMemberMode.cs
Expand Up @@ -20,15 +20,15 @@ public enum DataMemberMode
Assign,

/// <summary>
/// Only valid for a property / field that has a class or struct type.
/// When restored, instead of recreating the whole class or struct,
/// Only valid for a property / field that return a class, no strings, primitives or value types.
/// When restored, instead of recreating the whole class,
/// the members are independently restored. When the property / field
/// is not writeable this is the default.
/// </summary>
Content,

/// <summary>
/// Only valid for a property / field that has an array type of a
/// Only valid for a property / field that has an array type of
/// some value type. The content of the array is stored in a binary
/// format encoded in base64 style.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions sources/core/Stride.Core/PropertyKey.cs
Expand Up @@ -42,6 +42,7 @@ protected PropertyKey([NotNull] string name, Type propertyType, Type ownerType,
/// <summary>
/// Gets the name of this key.
/// </summary>
[DataMember]
public string Name { get; protected set; }

/// <summary>
Expand Down