Add Grpc.Tools test using protobuf-net RuntimeModel.GetSchema() output

[mythz]

Hey Marc,

This includes a simple test project which tries to use ASP.NET Core's Grpc.Tools with the .proto output from RuntimeTypeModel.GetSchema which uses .NET's built-in Decimal, DateTime and Guid types which emits the following reference to bcl.proto:

import "protobuf-net/bcl.proto"; // schema for protobuf-net's handling of core .NET types

I'm not sure if it's an issue with protobuf-net or whether Grpc.Tool code-gen utility just doesn't support external references as when I add a local copy of protobuf-net/bcl.proto the syntax highlighting support in JetBrains Rider shows that the reference is correct, i.e. .bcl.Decimal no longer has syntax errors, but dotnet build fails with:

  Restore completed in 18.62 ms for C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj.
protobuf-net/bcl.proto : error : File not found. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]
Protos/services.proto(3,1): error : Import "protobuf-net/bcl.proto" was not found or had errors. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]
Protos/services.proto(23,5): error : ".bcl.Decimal" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]       
Protos/services.proto(25,5): error : ".bcl.DateTime" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]      
Protos/services.proto(26,5): error : ".bcl.TimeSpan" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]      
Protos/services.proto(28,5): error : ".bcl.Guid" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]
Protos/services.proto(31,5): error : ".bcl.DateTime" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]      
Protos/services.proto(32,5): error : ".bcl.TimeSpan" is not defined. [C:\src\protobuf-net.Grpc\tests\protobuf-net.Grpc.Tool\protobuf-net.Grpc.Tool.csproj]      

Build FAILED.

Where it can't find the reference to protobuf-net/bcl.proto, it also doesn't work if I move it to bcl.proto suggesting it either doesn't support external references or it's incompatible with protobuf-net output.

Is there a way to make clients using ASP.NET gRPC's Grpc.Tools with protogen output? e.g. I can make it build if I inline the bcl.proto types and change the references of .bcl.Decimal to Decimal but I'm not sure if protobuf deserialization supports it, e.g. whether it can deserialize types sent from clients generated using .bcl.Decimal and inlined Decimal type?

Otherwise is there a global configuration we can use to specify that protobuf-net use gRPC's built-in Duration and Timestamp formats (in order to avoid the bcl.proto external reference). I'm aware users can individually specify it per property:

[ProtoMember(n, DataFormat = DataFormat.WellKnown)]

But that's error prone to rely on users specifying each time and most of the time the models don't have any external references, i.e. they use [DataMember(Order)] instead of [ProtoMember].

Otherwise is the recommendation that C# clients can't use Grpc.Tools to generate their clients and they should use protobuf-net's generator instead? Do you know if Google's code generation tools for their other languages can handle the external bcl.proto reference?

Do you know if this Grpc.Tools incompatibility with protobuf-net bcl.proto a known issue and should I be filing an issue with the ASP.NET project instead?

[mgravell]

Grpc.Tools is the Google package, right? It should be using regular protoc, in which case: everything should "just work" - I will check later today (something to do on a flight, yay!). My first guess is something about whether the right files are in the build output.

Re the global switch idea; this is something I've been considering for "some time". I'm open to suggestions. I was thinking of something like an assembly-level attribute, but that still needs a reference.

[mgravell]

Here you go:

     <ItemGroup>
-      <Protobuf Include="Protos\services.proto" GrpcServices="Client" />
+      <Protobuf Include="Protos/**/*.proto" GrpcServices="Client" ProtoRoot="Protos" />
     </ItemGroup>

• the ProtoRoot tells it where to start looking for imports
• the **/*.proto makes sure we generate the types from bcl.proto

I got this from https://github.com/grpc/grpc/blob/master/src/csharp/BUILD-INTEGRATION.md, FYI - good document

[mgravell]

Note: this still uses the hella awkward layout for dates/times, and frankly I strongly recommend using timestamp/duration instead. The only reason protobuf-net doesn't use these by default is because of backwards compat (they didn't exist when protobuf-net had to make something up).

Note also that the v3 version of protobuf-net's own schema tools should handle timestamp/duration natively by default

[mythz]

Brilliant, this works nicely:

<Protobuf Include="Protos/**/*.proto" GrpcServices="Client" ProtoRoot="Protos" />

And thanks for the informative link to BUILD-INTEGRATION.md.

Grpc.Tools is the Google package, right? It should be using regular protoc,

I did some more research into the tooling and potential workarounds, yeah it's basically a MS Builds tool wrapper around the native protoc with x86/x64 versions for linux/macos/windows.

I could get it working with the native protoc by either copying the protobuf-net/ folder to its include/ folder, but strangely this approach didn't work when trying to copy it into the native/include folder of the grpc.tools NuGet packages folder.

It also works as expected by running it on the command-line by specifying each file:

protoc.exe -I=Protos --csharp_out=. Protos\protobuf-net\bcl.proto Protos\services.proto

Where it generates a *.cs source file for each proto.

I strongly recommend using timestamp/duration instead.

I'd much prefer to use grpc's built-in timestamp/duration types as well but I'm not able to use [ProtoMember(n, DataFormat = DataFormat.WellKnown)] in our DTOs as they don't have references to any implementation Assemblies.

Note also that the v3 version of protobuf-net's own schema tools should handle timestamp/duration natively by default

I'm using GetSchema(ProtoSyntax.Proto3) to generate the types, i.e:

TypeModel.GetSchema(null /*all types*/, ProtoSyntax.Proto3);

But it's still using bcl.proto DateTime types, how do we get it to use grpc's Date Type?

[mgravell]

I meant the other way around - .proto to C#. The C# to .proto is controlled by the attribute currently. I totally hear you: you want to be able to specify that in a way that doesn't involve the reference. So: what options *would* work for configuring this?

…

[mythz]

I'd imagine a configuration option on RunTimeModel, e.g. UseNativeGrpcTypes=true or a customization option we can pass in GetSchema() to ideally avoid needing the external bcl.proto reference.

[mgravell]

or a customization option we can pass in GetSchema()

The problem here isn't GetSchema and it can't be "solved" there, because the entire point is that the layout of the two options is completely different. When it is using the google-compatible layout, GetSchema already emits the thing you want here.

I'd imagine a configuration option on RunTimeModel, e.g. UseNativeGrpcTypes=true

The problem there is that we just end up in exactly the same position for some hypothetical "next" set of types. It kinda needs to be specific to the types. I'm loathe to add a bunch of bools here, but perhaps something like

UseWellKnownTypesByDefault = WellKnownTypes.DateTime | WellKnownTypes.TimeSpan;

(and explicitly not an All option, because then we get into the same position again when something else gets added)

thoughts?

[mythz]

Yep, this works:

UseWellKnownTypesByDefault = WellKnownTypes.DateTime | WellKnownTypes.TimeSpan;

[mythz]

Curious to know if you've had time to look into this further? I've spent some generating gprc for different languages and the additional bcl.proto artifact kind of feels like an ugly concession for having used .NET.

IMO the ideal use-case for this feature would be something like: do a best effort to use gRPC defaults/conventions to avoid needing bcl.proto. So a single flag that we can configure on both client/server would be ideal. I can see why you wouldn't want an All option, but what if it was a (server) version number, so that any types supported before that version number would use the "non bcl.proto" option. (Behind the scenes it could configure a preset combination of enum flags as above).

So when a well known gRPC type exists (DateTime/TimeSpan) it'd use that, when it doesn't exist my preference would be to use an opinionated convention that can survive round trips, so for Guid/Decimal it'd just use a string, i.e. ToString() and InvariantCulture when localization dependent.

Not sure what you think about this behavior? But IMO avoiding needing an external reference to bcl.proto would be the preference of most new gRPC/protobuf-net users.

[mgravell]

Agree with you on all points. Especially guids. Holy cow guids are a cluster.

Also agree on the version idea; that was another option that I've been mulling for a while. Possibly an enum like:

// controls the conventions used by the engine; when starting
// new work, it is recommended to use the highest version available;
// Care must be used when changing between versions as many convention
// changes are data-incompatible; read the comments on each to
// see what has changed
public enum BehaviorDefaults {
    Legacy, // use legacy behaviors, including datetime/timespan via bcl.proto
    V3_0, // use well-known conventions where possible - guids as string, datetime as timestamp, timespan as duration (etc, being explicit)
    // TODO: future combinations etc here, always as at least a "minor" (not "revision")
}

I haven't touched it yet, but I can look at it next. I haven't formally released 3.0, so this would seem an ideal thing to make it into 3.0 rather than waiting for 3.1

[mythz]

Yep being able to specify an explicit version behavior like this would be perfect.

[mgravell]

With the protobuf-net changes soon to drop (CompatibilityLevel) - is this still needed?

[mythz]

Yep don't need this anymore thx to CompatibilityLevel.Level300 avoiding needing reference to bcl.proto, there still may be an issue with inheritance in protoc but the using Flattened Hierarchy's works around it.