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.

Sign up for GitHub

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

Jump to bottom

protobuf-net inheritance incompatible with Google's Protoc types #50

Open
mythz opened this issue Dec 23, 2019 · 21 comments
Open

protobuf-net inheritance incompatible with Google's Protoc types #50

mythz opened this issue Dec 23, 2019 · 21 comments

Comments

@mythz
Copy link
Contributor

mythz commented Dec 23, 2019

Hi Marc,

I tried to send a new Pull Request but as my previous PR hasn't been merged yet, my new PR was added to the previous one instead of creating a new one.

Anyway the relevant commit is:

25ee825

Where I've added tests to test protobuf-net inheritence and compatibility with Google's protoc and grpc code-gen for C#.

There's a few issues, tested with these models:

[ProtoContract]
[ProtoInclude(10, typeof(Sub))]
[ProtoInclude(11, typeof(GenericBase<>))]
public abstract class Base
{
    [ProtoMember(1)]
    public string String { get; set; }
    [ProtoMember(2)]
    public Dictionary<string,string> StringDictionary { get; set; }
}

[ProtoContract]
public class Sub : Base
{
    [ProtoMember(1)]
    public long Long { get; set; }
}

[ProtoContract]
public class Poco
{
    [ProtoMember(1)]
    public short Short { get; set; }
}

[ProtoContract]
[ProtoInclude(20, typeof(SubGeneric))]
public class GenericBase<T> : Base
{
    [ProtoMember(1)]
    public T Result { get; set; }
}

[ProtoContract]
public class SubGeneric : GenericBase<Poco>
{
    [ProtoMember(1)]
    public int Int { get; set; }
}

The basic inheritance test works where you can Serialize/Deserialize the simple Sub example in protobuf-net, but it partially fails when trying to deserialize the SubGeneric Type where it fails to deserialize the Base Types properties:

var from = new SubGeneric { 
    Int = 1, 
    String = "Base", 
    StringDictionary = new Dictionary<string, string> {
        {"A", "OK"}
    },
    Result = new Poco {
        Short = 2
    }
};

using var ms = new MemoryStream();
Serializer.Serialize(ms, from);
ms.Position = 0;
var to = Serializer.Deserialize<SubGeneric>(ms);

Assert.Equal(from.Result.Short, to.Result.Short);
Assert.Equal(from.Int, to.Int);
Assert.Equal(from.String, to.String); //FAIL to.String == null
Assert.Equal(from.StringDictionary["A"], to.StringDictionary["A"]); //FAIL to.StringDictionary == null

Protoc Inheritance Incompatibility

I've then taken the .proto generated from typeModel.GetSchema() schema to generate the protoc models using Google's protoc and grpc tools which whilst it generates all types does not provide anyway AFAICS to populate the base properties:

[Fact]
public void CanDeserializeInheritedTypesWithProtoc()
{
    var from = new Sub {
        Long = 1,
        // Cannot populate inherited properties: 
        // String = "Base", 
        // StringDictionary = new Dictionary<string, string> {
        //     {"A", "OK"}
        // }
    };

    using var ms = new MemoryStream();
    from.WriteTo(ms);
    ms.Position = 0;

    var to = Sub.Parser.ParseFrom(ms);

    Assert.Equal(from.Long, to.Long);
}

From the inheritence.proto we can see that the Base type has the container for all the sub types that inherits it, but I don't see how that's meant to work with gRPC in practice? as when you define a Service that sends a Sub or SubGeneric it only generates an API that lets you pass an instance of Sub in, not the Base type which contains all the Sub Type info:

rpc GetSub(Sub) returns (Sub) {}

Otherwise the test from protoc Sub -> protoc Sub is able to serialize its direct property, however it doesn't work when trying to deserialize protoc Sub -> protobuf.net Sub:

[Fact]
public void CanDeserializeProtocInheritedTypesWithProtobufNet()
{
    var from = new Sub { 
        Long = 1,
        // Cannot populate inherited properties: 
        // String = "Base", 
        // StringDictionary = new Dictionary<string, string> {
        //     {"A", "OK"}
        // }
    };
    
    using var ms = new MemoryStream();
    from.WriteTo(ms);
    ms.Position = 0;
    //throws System.InvalidOperationException : It was not possible to prepare a serializer for: ProtobufNet.Grpc.Test.Integration.Base
    var to = Serializer.Deserialize<Integration.Sub>(ms); 
    
    Assert.Equal(from.Long, to.Long);
}

I'm assuming this scenario is meant to be compatible as it's required for protobuf-net.Grpc to work with protoc generated clients, but I'm not seeing where I'm doing inheritance wrong as from all relevant docs the Sub types are meant to be annotated on the Base type but this doesn't look like it translates well in protoc generated clients.
@mgravell
Copy link
Member

mgravell commented Dec 24, 2019 via email

@mythz
Copy link
Contributor Author

mythz commented Dec 24, 2019

The issues with null: can't be resolved. Protobuf has no concept of null and there is no way to express it. It will forever be an ambiguous scenario.

Not sure what you mean, the null failures is due to protobuf-net not being able to deserialize the SubGeneric type fully, i.e. what's needed to change for the SubGeneric example to pass so all properties are deserialized?

When using protoc, you won't be using inheritance, but assigning the "oneof" field the correct message that maps to the conceptual sub-type should work fine. Is this not working somehow?

Here's the protoc generated Sub class which doesn't contain any of its Base Type fields. The oneof Types are on the Base class not the Sub class, so when a Service accepts and returns the Sub type how can it access its Base properties?

@mgravell
Copy link
Member

mgravell commented Dec 24, 2019 via email

@JohnGalt1717
Copy link

Does proto3 not support optional at least behind a flag now?

@mgravell
Copy link
Member

@JohnGalt1717 it does, and protobuf-net's tooling recognizes it and does the right things; that doesn't change much, though - all that changes is that a client can track definite assignment - it does not change the reality of null handling in protobuf

I'm conscious that I need to catch up on this particular issue, though, as a lot of things have moved in the interim

@JohnGalt1717
Copy link

Any movement on getting inheritance on interfaces and dtos working? I have a ton of duplicate code at this point because of it not functioning.

@mgravell
Copy link
Member

mgravell commented Aug 18, 2020 via email

@JohnGalt1717
Copy link

?

Here's an example that throws an error (very unhelpful error too, would be nice if all of the Proxy NotSupported errors had a description so it was easier to diagnose what it doesn't like))

public class ListResponse {
public ICollection Items {get; set;}
}

Also:

public class AtomRef {
Fields
}

public class Atom : AtomRef {
Somemore Fields carrying on the DataMember(Order = xxx) where AtomRef left off.
}

In the Atom example all of the fields defined in AtomRef won't be serialized. No error, just nothing serialized. I can't create an inherited response that works.

And you get a NotSupportedException (again no discription of what actually failed) if you use a ServiceContract that inherits from a Generic Interface.

@mgravell
Copy link
Member

mgravell commented Aug 18, 2020 via email

@JohnGalt1717
Copy link

@mgravell Looking at the source it appears that it knows what isn't supported pretty well so it would just be a matter of passing a message telling the dev what failed "Inheriting generic ServiceContract interfaces is not supported" etc.

@behnam-basketasia
Copy link

@mgravell i have same problem with @mythz
i think the problem exists still
i want to use generic abstract class by inheritence
would you provide an example that works for us?

i try to use same scenario, but Result does not change

[ProtoContract]
[ProtoInclude(20, typeof(SubGeneric))]
public class GenericBase<T> : Base
{
    [ProtoMember(1)]
    public T Result { get; set; }
}

[ProtoContract]
public class SubGeneric : GenericBase<Poco>
{
    [ProtoMember(1)]
    public int Int { get; set; }
}

@veronicabiondi
Copy link

veronicabiondi commented May 3, 2022 •
edited
Loading

@mgravell We have been using grpc services. We have been relying on proto files and for the known reasons, the idea of relying on .Net types is very attractive. We have a number of POC's in progress. We used protobuf-net for very simple types, where No Inheritance was involved. It worked.

However, I can't get it to work with inheritance. I tried the following model:

    **[ProtoContract]
    [ProtoInclude(1, typeof(SuccessResult<>))]
    public abstract class Result<T>
    {
        [ProtoMember(2)]
        public virtual T Value { get;  set; }
        [ProtoMember(3)]
        public abstract IEnumerable<ErrorMessage> Errors { get; }
        [ProtoMember(4)]
        public abstract  int StatusCode { get; }
        [ProtoMember(5)]
        public bool IsSuccessful => StatusCode >= 200 && StatusCode < 300;
    }**

   [ProtoContract]
   public class SuccessResult<T> : Result<T>
   {
       public SuccessResult(T t)
       {
           Value = t;
       }

       [ProtoMember(3)]
       public  override IEnumerable<ErrorMessage> Errors => new List<ErrorMessage>();

       [ProtoMember(4)]
       public override   int StatusCode => 200;

       [ProtoMember(3)]
       public override   T Value { get;  set; }
   }

The grpc service:

[ServiceContract(Name = "gRPCService.Services.CreateEntityAsync")]
  public interface IEntityService
  {
      [OperationContract]
      Task<Result<CreateEntityResponseMessage>> CreateEntityAsync(CreateEntityRequestMessage request);
  }
  
  public class EntityService : IEntityService
  {
    
      public async Task<Result<CreateEntityResponseMessage>> CreateEntityAsync(CreateEntityRequestMessage request)
      {
          Task<Result<CreateEntityResponseMessage>> task3 = Task<Result<CreateEntityResponseMessage>>.Factory.StartNew(() =>
          {
              return new SuccessResult<CreateEntityResponseMessage>(new CreateEntityResponseMessage()
              {
                  EntityId = 1,
                  Code = request.Code,
                  Label = request.Label
              });

          });
       
          return await task3;
      }
  }

The client :

   **var request = new CreateEntityRequestMessage()
                {
                    Code = cmd.Code,
                    ContractSize = cmd.ContractSize,
                    LocalCurrencyId = cmd.LocalCurrencyId,
                    IsIncluded = cmd.IsIncluded,
                    Label = cmd.Label
                };
                ProtoBuf.Grpc.Client.GrpcClientFactory.AllowUnencryptedHttp2 = true;
             
                using var channel = GrpcChannel.ForAddress("http://localhost:5064");

                var client = channel.CreateGrpcService<IEntityService>();
                var response = await client.CreateEntityAsync(request);**

It always throws an exception. Then I removed generics and tried with plain inheritance. No generic or abstract classes. But plain inheritance model. Is there something I'm doing wrong? I would really appreciate your help. First POC worked. Second was more advanced but I'm stuck!!!

@mgravell
Copy link
Member

mgravell commented May 3, 2022

"It always throws an exception."

What exception? what exactly does it say? I wonder if this is mostly an open generics problem, with the typeof(SuccessResult<>)

@veronicabiondi
Copy link

Many thanks for coming back so quickly. I was silly not to include the exception on my previous message, here it goes:

Grpc.Core.RpcException: Status(StatusCode="Unknown", Detail="Exception was thrown by handler.")
at ProtoBuf.Grpc.Internal.Reshape.UnaryTaskAsyncImpl[TRequest,TResponse](AsyncUnaryCall1 call, MetadataContext metadata, CancellationToken cancellationToken) in /_/src/protobuf-net.Grpc/Internal/Reshape.cs:line 289 at RestService.RestController.Get(CreateEntityCommand cmd) in C:\Projects\gRPCService\RestService\RestController.cs:line 38 at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask1 actionResultValueTask)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
at Microsoft.AspNetCore.Routing.EndpointMiddleware.g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

@veronicabiondi
Copy link

veronicabiondi commented May 3, 2022 •
edited
Loading

The client "lives" in a separate REST API.

```

[Route("api/[controller]")]
[ApiController]
public class RestController : ControllerBase
{
[HttpGet]
public async Task Get([FromQuery] CreateEntityCommand cmd)
{
try
{

            var request = new CreateEntityRequestMessage()
            {
                Code = cmd.Code,
                ContractSize = cmd.ContractSize,
                LocalCurrencyId = cmd.LocalCurrencyId,
                IsIncluded = cmd.IsIncluded,
                Label = cmd.Label
            };
            
            ProtoBuf.Grpc.Client.GrpcClientFactory.AllowUnencryptedHttp2 = true;
            using var channel = GrpcChannel.ForAddress("http://localhost:5064");
            var client = channel.CreateGrpcService<IEntityService>();
            var response = await client.CreateEntityAsync(request);
            return Ok(response);
        }
        catch (Exception ex)
        {
            throw;
        }
    }
}


If I try with plain objects (i.e passing a Request from the gRPC service, it works fine)

@veronicabiondi
Copy link

veronicabiondi commented May 3, 2022 •
edited
Loading

@mgravell it would be good to know if this is something that is not supported yet. My concern is also that inheritance with simple types also didn't work. We have lots of new gRPC services/REST API's coming up in the next few sprints and we are trying to make a decision if we can get away from proto files. Important factor is not only the complex types that we would like to be able to expose but if the above inheritance model can be supported. I have looked around and tried the suggested ways
for more than few days but It does not seem to be working.

@mgravell
Copy link
Member

mgravell commented May 3, 2022

Inheritance with simple types (meaning: not open generic types) - should work fine. I am open to exploring options to make your main example above work.

@veronicabiondi
Copy link

Simple types don't work either. About generic types, I think it's much more than "nice to have". It's almost a must. The reason is that usually when inheritance is used in types that we return from API's, when we subclass them, we also tend to make them reusable and hence the need for generics.

@veronicabiondi
Copy link

My new simplified model. Still not working. really, not sure what I'm doing wrong.

   [ProtoContract]
   [ProtoInclude(3, typeof(SuccessResult))]
   public class Result
   {
       [ProtoMember(2)]
       public IEnumerable<ErrorMessage> Errors { get; }
       [ProtoMember(4)]
       public int StatusCode { get; }
       [ProtoMember(5)]
       public bool IsSuccessful => StatusCode >= 200 && StatusCode < 300;
   }
   
   [ProtoContract]
   public class SuccessResult : Result
   {
       public SuccessResult(object t)
       {
           Value = t;
       }

       [ProtoMember(2)]
       public new int StatusCode => 200;

       [ProtoMember(3)]
       public object Value { get; protected set; }
   }
   

And the exception:

Error: response status is 500Response bodyDownloadGrpc.Core.RpcException: Status(StatusCode="Unknown", Detail="Exception was thrown by handler.") at ProtoBuf.Grpc.Internal.Reshape.UnaryTaskAsyncImpl[TRequest,TResponse](AsyncUnaryCall1 call, MetadataContext metadata, CancellationToken cancellationToken) in /_/src/protobuf-net.Grpc/Internal/Reshape.cs:line 289 at RestService.RestController.Get(CreateEntityCommand cmd) in C:\Projects\gRPCService\RestService\RestController.cs:line 35 at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited\|12_0(ControllerActionInvoker invoker, ValueTask1 actionResultValueTask) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope) at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope) at Microsoft.AspNetCore.Routing.EndpointMiddleware.g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger) at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext) at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider) at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

@mgravell
Copy link
Member

mgravell commented May 3, 2022 •
edited
Loading

There's a glitch in the main gRPC code (out of my hands), which I've been nagging them about for literally years, where serialization messages get swallowed. I imagine the problem here is the constructor. Try adding SkipConstructor = true to the sub-type's [ProtoContract(...)]. I'm not at a PC right now, but I'll check tomorrow to get the actual error. Alternatively, protobuf-net.BuildTools contains Roslyn anaylzers that can spot a wide range of common configuration errors.

@veronicabiondi
Copy link

Thanks. I would really like to use it but I've seen lot of issues coming up. I don't think that team is going to approve. I've spent days for a very basic scenario. Will check back at some point in the future. I think for now unfortunately proto files will stay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mgravell @mythz @JohnGalt1717 @behnam-basketasia @veronicabiondi