Add gRPC JSON transcoding #40242

JamesNK · 2022-02-15T08:34:18Z

Moving code from https://github.com/aspnet/AspLabs/tree/main/src/GrpcHttpApi

Unit and integration tests - Not worth the time to review
Microsoft.AspNetCore.Grpc.HttpApi - main project
- / - public API
- /Internal/Binding - code related to discovering and binding grpc methods as RESTful endpoints
- /Internal/CallHandlers - entry code when executing gRPC JSON transcoding
- /Internal/Json - code for System.Text.Json to convert from protobuf to JSON. See https://developers.google.com/protocol-buffers/docs/proto3#json. Ignore Legacy.cs. Copied from Google.Protobuf and will be replaced
- /Internal - various helper methods. Also has callcontext and streamwriter which are used when executing gRPC methods as RESTful endpoints
Microsoft.AspNetCore.Grpc.Swagger - swashbuckle integration. Populates ApiDescriptionProvider, as well as customizing how swashbuckle generates schemas for protobuf types and swagger documentation from generated XML
- / - public API
- /Internal/XmlComments - custom swagger documentation from generated XML
- /Internal - everything else

eng/Dependencies.props

...f/Microsoft.AspNetCore.Grpc.Microbenchmarks/Microsoft.AspNetCore.Grpc.Microbenchmarks.csproj

...Core.Grpc.HttpApi.IntegrationTests/Microsoft.AspNetCore.Grpc.HttpApi.IntegrationTests.csproj

eng/Build.props

src/Grpc/Interop/test/InteropTests/InteropTests.csproj

eng/Build.props

...Core.Grpc.HttpApi.IntegrationTests/Microsoft.AspNetCore.Grpc.HttpApi.IntegrationTests.csproj

.../test/Microsoft.AspNetCore.Grpc.Swagger.Tests/Microsoft.AspNetCore.Grpc.Swagger.Tests.csproj

src/Grpc/HttpApi/test/testassets/IntegrationTestsWebsite/IntegrationTestsWebsite.csproj

src/Grpc/HttpApi/perf/Microsoft.AspNetCore.Grpc.Microbenchmarks/DefaultCoreConfig.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/GrpcHttpApiServiceExtensions.cs

...ttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/HttpApiProviderSeviceBinder.cs

...i/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/ReflectionServiceInvokerResolver.cs

...Api/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/CallHandlers/CallHandlerDescriptorInfo.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/GrpcProtocolConstants.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/HttpApiServerCallContext.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/HttpContextStreamWriter.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Json/MessageConverter.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Microsoft.AspNetCore.Grpc.HttpApi.csproj

.../test/Microsoft.AspNetCore.Grpc.Swagger.Tests/Microsoft.AspNetCore.Grpc.Swagger.Tests.csproj

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.Swagger/Microsoft.AspNetCore.Grpc.Swagger.csproj

...ttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/HttpApiProviderSeviceBinder.cs

...tpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/HttpApiServiceMethodProvider.cs

captainsafia · 2022-02-22T04:12:08Z

...ttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/CallHandlers/UnaryServerCallHandler.cs

+
+        if (serverCallContext.Status.StatusCode != StatusCode.OK)
+        {
+            throw new RpcException(serverCallContext.Status);


Is there a message that can be provided here?

dougbu

@wtgodbe please double-check the msbuild goop I added. It's pretty much as we discussed -- I think.

src/BuildAfterApp/BuildAfterApp.csproj

dougbu · 2022-02-28T04:39:16Z

src/Grpc/HttpApi/Directory.Build.props

+    <!-- Workaround lack of Linux MUSL x64 support. -->
+    <ExcludeFromBuild
+        Condition=" '$(TargetOsName)' == 'linux-musl' and '$(TargetArchitecture)' == 'x64' ">true</ExcludeFromBuild>


@JamesNK I'm not sure of the bug here but the tooling fails to start on Linux MUSL x64.

Grpc.Tools has some native components. Might not support architecture?

Do the gRPC integration tests have the same issue? They also use Grpc.Tools.

Not sure what you mean by "gRPC integration tests". If that's InteropTests, then no, they're fine because we don't run or build tests on Linux MUSL x64. Hit the problem here because the folder includes implementation projects and I hadn't got the exclusions in BuildAfterApp.csproj working right. (Yeah, I discovered a problem because something else was busted 😺)

Put another way, I left this here as defence-in-depth and a reminder.

src/Grpc/HttpApi/src/Shared/Server/DuplexStreamingServerMethodInvoker.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Json/AnyConverter.cs

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Json/FieldMaskConverter.cs

BrennanConroy · 2022-02-28T18:59:31Z

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Json/Int64Converter.cs

+    {
+        if (reader.TokenType == JsonTokenType.String)
+        {
+            return long.Parse(reader.GetString()!, CultureInfo.InvariantCulture);


Is support for string numbers really needed?

It's a rule in gRPC <-> JSON transcoding. 64 bits don't fit into a JavaScript's 50ish bit number. So send as a string instead.

Not a fan but need to support it.

src/BuildAfterApp/BuildAfterApp.csproj

wtgodbe

MSBuild goop looks good, other than the name of the .csproj

dougbu · 2022-03-02T01:42:06Z

@JamesNK I rebased on 'main' to handle the AspNetCore.sln conflicts. Will address @wtgodbe's renaming suggestion later today or tomorrow.

captainsafia

LGTM YOLO

...Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs

halter73 · 2022-03-24T17:59:26Z

src/Grpc/Interop/test/testassets/InteropWebsite/TestServiceImpl.cs

@@ -50,9 +47,10 @@ public override async Task StreamingOutputCall(StreamingOutputCallRequest reques

        foreach (var responseParam in request.ResponseParameters)
        {
+            // TODO(JamesNK): Remove nullable override after Grpc.Core.Api update


Can we do this now after the 2.40.0 -> 2.43.0 updates in Version.props?

No, it requires 2.45.0

halter73 · 2022-03-24T18:04:46Z

...rc/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Binding/HttpApiProviderSeviceBinder.cs

+            else
+            {
+                // Consider setting to enable mapping to methods without HttpRule
+                // AddMethodCore(method, method.FullName, "GET", string.Empty, string.Empty, methodDescriptor);


Should there be a log when an HttpRule is not found similar to when a MethodDescriptor is not found?

No, it's normal to have a gRPC method that you don't want a RESTful version for. They don't have a HttpRule defined on them by the user.

halter73 · 2022-03-24T19:08:06Z

...rc/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/UnaryServerCallHandler.cs

+        if (response == null)
+        {
+            // This is consistent with Grpc.Core when a null value is returned
+            throw new RpcException(new Status(StatusCode.Cancelled, "No message returned from method."));


Out of curiosity, did we ever come to a conclusion about grpc/grpc-dotnet#1555?

No. Next 1:1 with Jan I'll bring it up again.

halter73 · 2022-03-24T19:10:11Z

...JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/ErrorMessageHelper.cs

+    {
+        if (includeExceptionDetails ?? false)
+        {
+            return message + " " + CommonGrpcProtocolHelpers.ConvertToRpcExceptionMessage(exception);


What's wrong with exception.ToString()? Are we trying to avoid stack traces?

halter73 · 2022-03-24T19:16:58Z

...ranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/FieldMaskConverter.cs

+        {
+            throw new InvalidOperationException("Expected string value for FieldMask.");
+        }
+        // TODO: Do we *want* to remove empty entries? Probably okay to treat "" as "no paths", but "foo,,bar"?


If we didn't remove empty entries, "" would give a single path and "," would give two? I think removing empty entries probably makes sense, but I don't know the spec.

This field mask logic matches what is in the Google.Protobuf serializer. I'll add to the todo that we should follow their lead.

halter73 · 2022-03-24T19:17:29Z

...ranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/FieldMaskConverter.cs

+
+    public override void Write(Utf8JsonWriter writer, TMessage value, JsonSerializerOptions options)
+    {
+        var paths = (IList<string>)value.Descriptor.Fields[FieldMask.PathsFieldNumber].Accessor.GetValue(value);


Should we remove empty entries from paths then?

This field mask logic matches what is in the Google.Protobuf serializer. I'll add to the todo that we should follow their lead.

halter73 · 2022-03-24T19:30:30Z

....AspNetCore.Grpc.JsonTranscoding.IntegrationTests/Infrastructure/ForwardingLoggerProvider.cs

+
+        public IDisposable BeginScope<TState>(TState state)
+        {
+            return null!;


Soon we won't need this !! dotnet/runtime#66960

JamesNK · 2022-03-26T11:08:05Z

@dougbu There is a project - Microsoft.AspNetCore.Grpc.Swagger - in this PR that isn't ready yet to be published. It will hopefully be ready in a couple of previews.

I set IsPackable and IsShipping to false. Is that the right way to exclude it?

dougbu · 2022-03-26T18:28:36Z

I set IsPackable and IsShipping to false. Is that the right way to exclude it?

Either works. $(IsPackable) controls whether the build creates a .nupkg at all and $(IsShipping) controls package versioning -- non-shipping packages are created but automatically not included in a release.

JamesNK requested review from dougbu, and Pilchie as code owners Feb 15, 2022

Pilchie added the area-grpc label Feb 15, 2022

JamesNK force-pushed the jamesnk/grpctranscoding branch from f309f02 to b3c0dab Compare Feb 15, 2022

dougbu reviewed Feb 15, 2022

View changes

eng/Dependencies.props Outdated Show resolved Hide resolved

...f/Microsoft.AspNetCore.Grpc.Microbenchmarks/Microsoft.AspNetCore.Grpc.Microbenchmarks.csproj Outdated Show resolved Hide resolved

JamesNK force-pushed the jamesnk/grpctranscoding branch from df0a1cb to 3e5f59e Compare Feb 16, 2022

dougbu reviewed Feb 17, 2022

View changes

...Core.Grpc.HttpApi.IntegrationTests/Microsoft.AspNetCore.Grpc.HttpApi.IntegrationTests.csproj Outdated Show resolved Hide resolved

eng/Build.props Outdated Show resolved Hide resolved

dougbu reviewed Feb 17, 2022

View changes

src/Grpc/Interop/test/InteropTests/InteropTests.csproj Outdated Show resolved Hide resolved

dougbu reviewed Feb 17, 2022

View changes

rafikiassumani-msft requested review from ReubenBond, BrennanConroy and captainsafia Feb 17, 2022

BrennanConroy reviewed Feb 17, 2022

View changes

src/Grpc/HttpApi/perf/Microsoft.AspNetCore.Grpc.Microbenchmarks/DefaultCoreConfig.cs Outdated Show resolved Hide resolved

src/Grpc/HttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/GrpcHttpApiServiceExtensions.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Feb 17, 2022

View changes

BrennanConroy reviewed Feb 18, 2022

View changes

dougbu reviewed Feb 18, 2022

View changes

captainsafia reviewed Feb 22, 2022

View changes

...ttpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/HttpApiProviderSeviceBinder.cs Outdated Show resolved Hide resolved

captainsafia reviewed Feb 22, 2022

View changes

...tpApi/src/Microsoft.AspNetCore.Grpc.HttpApi/Internal/Binding/HttpApiServiceMethodProvider.cs Outdated Show resolved Hide resolved

captainsafia reviewed Feb 22, 2022

View changes

dougbu self-assigned this Feb 23, 2022

dougbu mentioned this pull request Feb 23, 2022

Code gen #40383

Closed

dougbu force-pushed the jamesnk/grpctranscoding branch from 2675ba1 to 25b495e Compare Feb 27, 2022

JamesNK requested a review from wtgodbe as a code owner Feb 27, 2022

dougbu reviewed Feb 28, 2022

View changes

BrennanConroy reviewed Feb 28, 2022

View changes

wtgodbe reviewed Feb 28, 2022

View changes

src/BuildAfterApp/BuildAfterApp.csproj Outdated Show resolved Hide resolved

wtgodbe approved these changes Feb 28, 2022

View changes

dougbu force-pushed the jamesnk/grpctranscoding branch from dbb5e2c to 39758f4 Compare Mar 2, 2022

captainsafia approved these changes Mar 23, 2022

View changes

BrennanConroy approved these changes Mar 23, 2022

View changes

...Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs Outdated Show resolved Hide resolved

JamesNK unassigned dougbu Mar 24, 2022

halter73 reviewed Mar 24, 2022

View changes

halter73 approved these changes Mar 24, 2022

View changes

JamesNK added 9 commits Mar 26, 2022

Add gRPC JSON transcoding

19d5eb7

Project rename

132a2da

Address suggestions

6692e26

Apply API review changes

05c627a

XML comments

6cd4228

PR feedback

b2b7118

Fix UInt64Converter not using settings

8f9d6cf

Don't package swagger project

733c587

Don't package swagger project

c3174dd

JamesNK force-pushed the jamesnk/grpctranscoding branch from 1f8ef3e to c3174dd Compare Mar 26, 2022

JamesNK added 2 commits Mar 27, 2022

Fix build

80cefa4

Clean up

56b79a7

JamesNK merged commit b4b43e3 into main Mar 27, 2022
25 checks passed

JamesNK deleted the jamesnk/grpctranscoding branch Mar 27, 2022

msftbot bot added this to the 7.0-preview3 milestone Mar 27, 2022

dougbu removed this from the 7.0-preview3 milestone Apr 5, 2022

dougbu added this to the 7.0-preview4 milestone Apr 5, 2022

dotnet / aspnetcore Public

Add gRPC JSON transcoding #40242

Add gRPC JSON transcoding #40242

JamesNK commented Feb 15, 2022 •

edited

captainsafia Feb 22, 2022

dougbu left a comment

dougbu Feb 28, 2022

JamesNK Feb 28, 2022

dougbu Feb 28, 2022

dougbu Feb 28, 2022

BrennanConroy Feb 28, 2022

JamesNK Mar 1, 2022

wtgodbe left a comment

dougbu commented Mar 2, 2022

captainsafia left a comment

halter73 Mar 24, 2022

JamesNK Mar 24, 2022

halter73 Mar 24, 2022

JamesNK Mar 24, 2022

halter73 Mar 24, 2022

JamesNK Mar 24, 2022

halter73 Mar 24, 2022

JamesNK Mar 24, 2022

halter73 Mar 24, 2022

JamesNK Mar 25, 2022

halter73 Mar 24, 2022

JamesNK Mar 25, 2022

halter73 Mar 24, 2022

JamesNK commented Mar 26, 2022

dougbu commented Mar 26, 2022

dotnet / aspnetcore Public

Add gRPC JSON transcoding #40242

Add gRPC JSON transcoding #40242

Conversation

JamesNK commented Feb 15, 2022 • edited

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgodbe left a comment

dougbu commented Mar 2, 2022

captainsafia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Mar 26, 2022

dougbu commented Mar 26, 2022

JamesNK commented Feb 15, 2022 •

edited