googleapis / sdk-platform-java

Tooling and shared libraries for Cloud SDK for Java
https://cloud.google.com/java/docs/bom
Apache License 2.0
65 stars 52 forks source link

Handle `google.rpc.ErrorInfo` in HttpJson*Stub class #2237

Open JoeWang1127 opened 10 months ago

JoeWang1127 commented 10 months ago

Followup on #2229.

The generated HttpJsonEchoStub can't handle message type: google.rpc.ErrorInfo introduced by echo.proto (version 0.29.0). A easy integration test:

@Test
public void testEchoErrorDetails() {
  EchoErrorDetailsResponse response = httpjsonClient.echoErrorDetails(
      EchoErrorDetailsRequest
          .newBuilder()
          .setSingleDetailText("singleDetailText1774380934")
          .addAllMultiDetailText(new ArrayList<>())
          .build());
  assertThat(response.hasSingleDetail()).isEqualTo(true);
}

failed with the following error message:

Caused by: com.google.protobuf.InvalidProtocolBufferException: Cannot resolve type: type.googleapis.com/google.rpc.ErrorInfo
    at com.google.protobuf.util.JsonFormat$ParserImpl.mergeAny(JsonFormat.java:1511)

Possible solution: Add the message type in https://github.com/googleapis/sdk-platform-java/blob/003b993f7ad7cae8ae8c101e0ff147e517dcd83e/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java#L1279-L1286

JoeWang1127 commented 10 months ago

In the possible solution part, the types are added recursively in TypeRegistry.Builder.add() method.

However, only two types serves as the "root" of this method, WaitRequest and WaitResponse, we need to understand the reason behind this before adding ErrorInfo into the registry.

lqiu96 commented 10 months ago

Took a look at some of the code in Gax and I believe it may be due to the ProtoOperationTransformers: https://github.com/googleapis/sdk-platform-java/blob/9d30ed9b05dd77348899d312117e3fb562e7e6bd/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/ProtoOperationTransformers.java#L130-L140

ServiceStubSettings transforms it from Any to the classes: https://github.com/googleapis/sdk-platform-java/blob/9d30ed9b05dd77348899d312117e3fb562e7e6bd/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/EchoStubSettings.java#L792-L795

I believe conversion from Any -> Class would require a TypeRegistry.

blakeli0 commented 10 months ago

I'm afraid this issue is more complicated than we expected. The new Showcase feature tests that we can deserialize an Any field to a concrete field in ErrorInfo, which is only used by the Error Details feature, and it was deferred last year. See b/260671246 for details. In short, we can not support this test for HttpJson stubs until Error Details feature is implemented for REGAPIC.

In the mean time, I think it's still worth it to add a showcase test for gRPC.

diegomarquezp commented 7 months ago

we can not support this test for HttpJson stubs until Error Details feature is implemented for REGAPIC

I'll be demoting this to p3

blakeli0 commented 1 month ago

Verified that adding ErrorInfo to the type registry should fix this issue, see https://github.com/googleapis/sdk-platform-java/pull/3093. However, we need to figure out where exact to add the additional types, either in Gax or generated layers.

Theoretically this issue can be tackled individually, but it will likely be resolved if the actionable ErrorDetails project is implemented for httpjson. Hence keeping this as a P3.