protobuf-net / protobuf-net

Protocol Buffers library for idiomatic .NET
Other
4.62k stars 1.05k forks source link

C#9 positional record types do not support inheritance #743

Open amoerie opened 3 years ago

amoerie commented 3 years ago

Hi, I'm creating this issue to track a problem that I've encountered while trying out protobuf-net, and that @mgravell already extensively answered on StackOverflow here: https://stackoverflow.com/questions/65123223/does-protobuf-net-support-c-sharp-9-positional-record-types

C# 9 positional record types ARE supported by protobuf-net, if you don't add any type annotations. This is OK for most scenarios, but it does not support inheritance, because inheritance requires the 'ProtoInclude' attribute.

Using positional records with inheritance, gives you the following Exception today:

System.InvalidOperationException: Tuple-based types cannot be used in inheritance hierarchies: ProtoBuf.Test.RecordTypeTests+PositionalRecordWithInher...

System.InvalidOperationException
Tuple-based types cannot be used in inheritance hierarchies: ProtoBuf.Test.RecordTypeTests+PositionalRecordWithInheritance
   at ProtoBuf.Internal.ThrowHelper.ThrowInvalidOperationException(String message, Exception innerException) in C:\git\protobuf-net\src\protobuf-net.Core\Internal\ThrowHelper.cs:line 55
   at ProtoBuf.Meta.MetaType.ThrowTupleTypeWithInheritance(Type type) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 167
   at ProtoBuf.Meta.MetaType.AddSubType(Int32 fieldNumber, Type derivedType, DataFormat dataFormat) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 147
   at ProtoBuf.Meta.MetaType.ApplyDefaultBehaviourImpl(CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 715
   at ProtoBuf.Meta.MetaType.ApplyDefaultBehaviour(CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 642
   at ProtoBuf.Meta.RuntimeTypeModel.FindOrAddAuto(Type type, Boolean demand, Boolean addWithContractOnly, Boolean addEvenIfAutoDisabled, CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\RuntimeTypeModel.cs:line 749
   at ProtoBuf.Meta.MetaType.ApplyDefaultBehaviourImpl(CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 652
   at ProtoBuf.Meta.MetaType.ApplyDefaultBehaviour(CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\MetaType.cs:line 642
   at ProtoBuf.Meta.RuntimeTypeModel.FindOrAddAuto(Type type, Boolean demand, Boolean addWithContractOnly, Boolean addEvenIfAutoDisabled, CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\RuntimeTypeModel.cs:line 749
   at ProtoBuf.Meta.RuntimeTypeModel.<GetServicesSlow>g__GetServicesImpl|88_0(RuntimeTypeModel model, Type type, CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\RuntimeTypeModel.cs:line 1028
   at ProtoBuf.Meta.RuntimeTypeModel.GetServicesSlow(Type type, CompatibilityLevel ambient) in C:\git\protobuf-net\src\protobuf-net\Meta\RuntimeTypeModel.cs:line 998
   at ProtoBuf.Meta.RuntimeTypeModel.GetSerializer[T]() in C:\git\protobuf-net\src\protobuf-net\Meta\RuntimeTypeModel.cs:line 961
   at ProtoBuf.Meta.TypeModel.TryGetSerializer[T](TypeModel model) in C:\git\protobuf-net\src\protobuf-net.Core\Meta\TypeModel.cs:line 1460
   at ProtoBuf.Meta.TypeModel.DeepClone[T](T value, Object userState) in C:\git\protobuf-net\src\protobuf-net.Core\Meta\TypeModel.cs:line 1521
   at ProtoBuf.Test.RecordTypeTests.CanRoundTripPositionalRecordWithInheritance() in C:\git\protobuf-net\src\protobuf-net.Test\RecordTypeTests.cs:line 44

If you want to use inheritance, you need to use [ProtoContract(SkipConstructor = true)], which circumvents the constructor altogether and uses - AFAIK - GetUninitializedObject and some reflection to assign the fields.

Ideally, it would seem to me, protobuf-net would support mixing C# record types with inheritance using the [ProtoInclude] attribute.

I have some time now, so I'll have a walk around the code to see if there is anything I can do to help with this issue (as it's important to me).

My only question is:

Completely optional, but if you can point me in some directions as to where a solution would be best implemented, that would also be helpful. If not, I'll try to find my own way. Thanks!

Also I'm not promising anything, this code base seems quite tricky..

mgravell commented 3 years ago

yes, I would absolutely be interested in - and consider - such a PR; it may be more nuanced than you expect, however, as I believe it is currently using .ctor construction rather than post-construction member mutation, so that would need to be changed - which might be a little gnarly.

amoerie commented 3 years ago

@mgravell you might not have seen my PR yet, I (may) have found a solution that is suspiciously simple.

It seems that positional records with only a ProtoInclude attribute can be supported if you just remove a few checks. Of course the question becomes: what were these checks safeguarding against, and are we now allowing certain scenarios that will throw exceptions further down the line?

mgravell commented 3 years ago

I'll comment on the PR; thanks for the typo fixes, btw :)

iSeiryu commented 3 years ago

C# 9 positional record types ARE supported by protobuf-net

Since which version? I am not using protobuf-net directly. I am using StackExchange.Redis.Extensions.Protobuf which references it. The current version it uses is 3.0.29.1881

image

I have a simple record

public sealed record GenderEntity(Guid Id, string Gender)

It serializes and saves it to Redis no problem. But when it tries to deserialize it it returns this error:

ProtoBuf.ProtoException: No parameterless constructor found for GenderEntity\n   
at ProtoBuf.Internal.ThrowHelper.ThrowProtoException(String message, Exception inner) in /_/src/protobuf-net.Core/Internal/ThrowHelper.cs:line 70\n  
at ProtoBuf.Meta.TypeModel.ThrowCannotCreateInstance(Type type, Exception inner) in /_/src/protobuf-net.Core/Meta/TypeModel.cs:line 1668\n   
at proto_2(State& , GenderEntity)\n   
at ProtoBuf.Internal.Serializers.SimpleCompiledSerializer`1.ProtoBuf.Serializers.ISerializer<T>.Read(State& state, T value)\n   
at ProtoBuf.ProtoReader.State.ReadMessage[TSerializer,T](SerializerFeatures features, T value, TSerializer& serializer)\n   
at ProtoBuf.ProtoReader.State.FillBuffer[TSerializer,T](SerializerFeatures features, TSerializer& serializer, T initialValue)\n   
at ProtoBuf.Serializers.RepeatedSerializer`2.ReadRepeated(State& state, SerializerFeatures features, TCollection values, ISerializer`1 serializer)\n   
at ProtoBuf.Serializers.RepeatedSerializer`2.ProtoBuf.Serializers.IRepeatedSerializer<TCollection>.ReadRepeated(State& state, SerializerFeatures features, TCollection values)\n   
at ProtoBuf.ProtoReader.State.<ReadAsRoot>g__ReadFieldOne|101_0[T](State& state, SerializerFeatures features, T value, ISerializer`1 serializer)\n   
at ProtoBuf.ProtoReader.State.ReadAsRoot[T](T value, ISerializer`1 serializer)\n   
at ProtoBuf.ProtoReader.State.DeserializeRoot[T](T value, ISerializer`1 serializer)\n   
at ProtoBuf.Serializer.Deserialize[T](Stream source)\n   
at StackExchange.Redis.Extensions.Protobuf.ProtobufSerializer.Deserialize[T](Byte[] serializedObject)\n   
at StackExchange.Redis.Extensions.Core.Implementations.RedisDatabase.GetAsync[T](String key, CommandFlags flag)

If I change my record to this it works just fine

public sealed record GenderEntity {
    public Guid Id { get; }
    public string Gender { get; }
}
mgravell commented 3 years ago

3.0.62

https://github.com/protobuf-net/protobuf-net/blob/main/docs/releasenotes.md#3062

On Wed, 16 Dec 2020, 18:16 iSeiryu, notifications@github.com wrote:

C# 9 positional record types ARE supported by protobuf-net

Since which version? I am not using protobuf-net directly. I am using StackExchange.Redis.Extensions.Protobuf which references it. The current version it uses is 3.0.29.1881

[image: image] https://user-images.githubusercontent.com/129137/102387312-36b2c400-3f9e-11eb-9e44-e27abb52ef42.png

I have a simple record

public sealed record GenderEntity(Guid Id, string Gender)

It serializes and saves it to Redis no problem. But when it tries to deserialize it it returns this error:

ProtoBuf.ProtoException: No parameterless constructor found for GenderEntity\n at ProtoBuf.Internal.ThrowHelper.ThrowProtoException(String message, Exception inner) in //src/protobuf-net.Core/Internal/ThrowHelper.cs:line 70\n at ProtoBuf.Meta.TypeModel.ThrowCannotCreateInstance(Type type, Exception inner) in //src/protobuf-net.Core/Meta/TypeModel.cs:line 1668\n at proto_2(State& , GenderLookup )\n at ProtoBuf.Internal.Serializers.SimpleCompiledSerializer1.ProtoBuf.Serializers.ISerializer<T>.Read(State& state, T value)\n at ProtoBuf.ProtoReader.State.ReadMessage[TSerializer,T](SerializerFeatures features, T value, TSerializer& serializer)\n at ProtoBuf.ProtoReader.State.FillBuffer[TSerializer,T](SerializerFeatures features, TSerializer& serializer, T initialValue)\n at ProtoBuf.Serializers.RepeatedSerializer2.ReadRepeated(State& state, SerializerFeatures features, TCollection values, ISerializer1 serializer)\n at ProtoBuf.Serializers.RepeatedSerializer2.ProtoBuf.Serializers.IRepeatedSerializer.ReadRepeated(State& state, SerializerFeatures features, TCollection values)\n at ProtoBuf.ProtoReader.State.g__ReadFieldOne|101_0[T](State& state, SerializerFeatures features, T value, ISerializer1 serializer)\n at ProtoBuf.ProtoReader.State.ReadAsRoot[T](T value, ISerializer1 serializer)\n at ProtoBuf.ProtoReader.State.DeserializeRoot[T](T value, ISerializer`1 serializer)\n at ProtoBuf.Serializer.Deserialize[T](Stream source)\n at StackExchange.Redis.Extensions.Protobuf.ProtobufSerializer.Deserialize[T](Byte[] serializedObject)\n at StackExchange.Redis.Extensions.Core.Implementations.RedisDatabase.GetAsync[T](String key, CommandFlags flag)

If I change my record to this it works just fine

public sealed record GenderEntity { public Guid Id { get; } public string Gender { get; } }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net/issues/743#issuecomment-746761688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMFR4XUOJ5GGB7CTU4DSVD2PDANCNFSM4U36EK5A .

iSeiryu commented 3 years ago

Just for the documentation purposes. They have a PR to bring in the latest protobuf-net: https://github.com/imperugo/StackExchange.Redis.Extensions/pull/360

mgravell commented 3 years ago

Fair enough, but any questions relating to that external package: need to be asked there.

On Thu, 17 Dec 2020, 15:54 iSeiryu, notifications@github.com wrote:

Just for the documentation purposes. They have a PR to bring in the latest protobuf-net: imperugo/StackExchange.Redis.Extensions#360 https://github.com/imperugo/StackExchange.Redis.Extensions/pull/360

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net/issues/743#issuecomment-747527281, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMCXDOSGLVLWLXD5YUTSVISTPANCNFSM4U36EK5A .

iSeiryu commented 3 years ago

I got rid of the package (basically copied their code to my app) and added the latest protobuf-net. Getting exactly the same issue. The current dlls are 3.0.73

263168 Dec  1 19:25 protobuf-net.Core.dll
259584 Dec  1 19:25 protobuf-net.dll
iSeiryu commented 3 years ago

I have this class.

using System.IO;
using ProtoBuf;

public class ProtobufSerializer {
  public byte[] Serialize(object item) {
    using var ms = new MemoryStream();

    Serializer.Serialize(ms, item);

    return ms.ToArray();
  }

  public T Deserialize<T>(byte[] serializedObject) {
    using var ms = new MemoryStream(serializedObject);

    return Serializer.Deserialize<T>(ms);
  }
}
var ser = new ProtobufSerializer();
var gender = new GenderEntity(default, "Male");
var bytes = ser.Serialize(gender);
Console.WriteLine("Serialized: " + bytes);

var deserializedGender = ser.Deserialize<GenderEntity>(bytes);
Console.WriteLine("Deserialized: " + deserializedGender.Id + "; " + deserializedGender.Gender);

An interesting part is if I define my record like this it works:

public sealed record GenderEntity(Guid Id, string Gender);

But if I add ProtoContract it crashes

[ProtoContract]
public sealed record GenderEntity(Guid Id, string Gender);
ProtoBuf.ProtoException: No parameterless constructor found for GenderEntity
      at ProtoBuf.Internal.ThrowHelper.ThrowProtoException(String message, Exception inner) in /_/src/protobuf-net.Core/Internal/ThrowHelper.cs:line 70
      at ProtoBuf.Meta.TypeModel.ThrowCannotCreateInstance(Type type, Exception inner) in /_/src/protobuf-net.Core/Meta/TypeModel.cs:line 1666
      at proto_2(State& , GenderEntity )
      at ProtoBuf.Internal.Serializers.SimpleCompiledSerializer`1.ProtoBuf.Serializers.ISerializer<T>.Read(State& state, T value)
      at ProtoBuf.ProtoReader.State.ReadAsRoot[T](T value, ISerializer`1 serializer)
      at ProtoBuf.ProtoReader.State.DeserializeRoot[T](T value, ISerializer`1 serializer)
      at ProtoBuf.Serializer.Deserialize[T](Stream source)
      at Blahblah.ProtobufSerializer.Deserialize[T](Byte[] serializedObject)

I have code that dynamically adds those attributes. I guess I need to exclude record types. Is it done by design?

mgravell commented 3 years ago

Ah, right: to use the simple handling of records - take away the [ProtoContract]

On Thu, 17 Dec 2020, 18:29 iSeiryu, notifications@github.com wrote:

I have this class. The Serializer class comes from

using System.IO;using ProtoBuf; public class ProtobufSerializer { public byte[] Serialize(object item) { using var ms = new MemoryStream();

Serializer.Serialize(ms, item);

return ms.ToArray();

}

public T Deserialize(byte[] serializedObject) { using var ms = new MemoryStream(serializedObject);

return Serializer.Deserialize<T>(ms);

} }

var ser = new ProtobufSerializer();var gender = new GenderLookup(default, "Male");var bytes = ser.Serialize(gender);Console.WriteLine("Serialized: " + bytes); var deserializedGender = ser.Deserialize(bytes);Console.WriteLine("Deserialized: " + deserializedGender.Id + "; " + deserializedGender.Gender);

An interesting part is if I define my record like this:

public sealed record GenderEntity(Guid Id, string Gender);

But if I add ProtoContract it crashes

[ProtoContract]public sealed record GenderEntity(Guid Id, string Gender);

ProtoBuf.ProtoException: No parameterless constructor found for GenderEntity at ProtoBuf.Internal.ThrowHelper.ThrowProtoException(String message, Exception inner) in //src/protobuf-net.Core/Internal/ThrowHelper.cs:line 70 at ProtoBuf.Meta.TypeModel.ThrowCannotCreateInstance(Type type, Exception inner) in //src/protobuf-net.Core/Meta/TypeModel.cs:line 1666 at proto_2(State& , GenderEntity ) at ProtoBuf.Internal.Serializers.SimpleCompiledSerializer1.ProtoBuf.Serializers.ISerializer<T>.Read(State& state, T value) at ProtoBuf.ProtoReader.State.ReadAsRoot[T](T value, ISerializer1 serializer) at ProtoBuf.ProtoReader.State.DeserializeRoot[T](T value, ISerializer`1 serializer) at ProtoBuf.Serializer.Deserialize[T](Stream source) at Blahblah.ProtobufSerializer.Deserialize[T](Byte[] serializedObject)

I have code that dynamically adds those attributes. I guess I need to exclude record types. Is it done by design?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net/issues/743#issuecomment-747617833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMH3VRR632KBTEO2HQ3SVJEYFANCNFSM4U36EK5A .

codymullins commented 6 months ago

Ah, right: to use the simple handling of records - take away the [ProtoContract]

@mgravell are you saying in order to use the short positional record types to exclude the [ProtoContract] attribute?

I'm using this library (thank you btw) for serialization in an event bus, and have lots of commands like below:

[ProtoContract]
public record DeploymentStarted([property: ProtoMember(1)] Guid Id, [property: ProtoMember(2)] Guid SiteId)
    : IProtoMessage;

I'm trying to decide how to move forward here, any feedback would be appreciated 🤞🏻

Screenshot 2024-02-28 at 8 30 48 AM