line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.83k stars 921 forks source link

Debug Form doesn't show for RPC in DocService #4284

Closed Dogacel closed 2 years ago

Dogacel commented 2 years ago

The DocService doesn't show any Debug Form options under the endpoint definitions:

image

I have tried the following:

    private val grpcServiceBuilder: GrpcServiceBuilder = GrpcService.builder()
        .supportedSerializationFormats(GrpcSerializationFormats.PROTO)
        .supportedSerializationFormats(GrpcSerializationFormats.PROTO_WEB)
        .supportedSerializationFormats(GrpcSerializationFormats.PROTO_WEB_TEXT)
        .supportedSerializationFormats(GrpcSerializationFormats.JSON)
        .supportedSerializationFormats(GrpcSerializationFormats.JSON_WEB)
        .enableUnframedRequests(true)

Also I have seen this issue: https://github.com/line/armeria/issues/1134

But no luck. Does anyone know what is happening and maybe we can improve the documentation for future users?

minwoox commented 2 years ago

Hey! Could you just try with this?

GrpcService.builder()
           .enableUnframedRequests(true)
           .addService(...)
           .build()

builder.supportedSerializationFormats(...) overwrites the values previously set, so I think only GrpcSerializationFormats.JSON_WEB is set by the code you pasted. Also, all serialization formats are set by default so you don't need to set them explicitly. 😉

Dogacel commented 2 years ago

@minwoox that did the trick, thanks. Which serialization format do we need to enable to use the debug form? Currently we are using .supportedSerializationFormats(GrpcSerializationFormats.PROTO) because JSON serialization does not run with proto3 optional fields. So enabling all can be risky for us.

Also, if I set enabledUnframedRequests(true), will it affect how the framed requests are handled? I guess not but I would like to double-check.

Finally: image

There is no example request body (I did not add any examples but I expected some default like bloomRPC did here - photo below). Is this expected or is there a way to enable this kind of thing in debug form? image

Update to the above statement: Seems like only optional and oneof fields are not rendered in the example body. It could be nice to see them somehow.

I can contribute to such a a feature and update the documentation to enable the debug form.

minwoox commented 2 years ago

Which serialization format do we need to enable to use the debug form?

It's GrpcSerializationFormats.JSON. https://github.com/line/armeria/blob/master/docs-client/src/lib/transports/grpc-unframed.ts#L21 (Sorry about that we don't have documentation about it. 😅 )

JSON serialization does not run with proto3 optional fields

Oops, I didn't know about it. Could you give us a link to it or provide a simple reproducible example so we can take a look at it?

if I set enabledUnframedRequests(true), will it affect how the framed requests are handled?

No, it's not. 😉 https://github.com/line/armeria/blob/master/grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcService.java#L100-L105

Seems like only optional and oneof fields are not rendered in the example body. It could be nice to see them somehow.

Oops, I didn't know about it. 😄 bloomRPC does render it by default? Anyway, it would be nice if we support those fields as well. 😄

ikhoon commented 2 years ago

because JSON serialization does not run with proto3 optional fields.

As far as I know, protobuf-jackson supports optional since 2.0.0 https://github.com/curioswitch/protobuf-jackson/issues/7 You can also set Gson as a json marshaller. https://github.com/line/armeria/blob/8df42aa225af877ef13781ec1b27c1fe0ce37883/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java#L71 If you encounter another issues except what we have known, please let us know.

Dogacel commented 2 years ago

because JSON serialization does not run with proto3 optional fields.

As far as I know, protobuf-jackson supports optional since 2.0.0 curioswitch/protobuf-jackson#7 You can also set Gson as a json marshaller.

https://github.com/line/armeria/blob/8df42aa225af877ef13781ec1b27c1fe0ce37883/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java#L71

If you encounter another issues except what we have known, please let us know.

I am not 100% sure but the comment might be outdated because this bug has been resolved recently (relatively). I will double check that and write back.

Oops, I didn't know about it. 😄 bloomRPC does render it by default? Anyway, it would be nice if we support those fields as well. 😄

Yeah, BloomRPC just dumps every possible field as an example. There is no difference between a oneof field and optional field tho but I the current implementation is good enough.

Oops, I didn't know about it. Could you give us a link to it or provide a simple reproducible example so we can take a look at it?

We have been using the library in the company's private project so I need some time to reproduce this in a sample project and come back to you.

minwoox commented 2 years ago

Yeah, BloomRPC just dumps every possible field as an example. There is no difference between a oneof field and optional field tho but I the current implementation is good enough.

But it would be awesome if we support those too. 😄 Are you interested in fixing that? 😆 I guess this is where we create the default examples now. 😉

Dogacel commented 2 years ago

this

Yep, I will take a look. Thanks 🙇

Dogacel commented 2 years ago

minwoox

Seems like BloomRPC is using a completely different serializer than JsonFormat.

We are not outputting any optional value because of the following logic inside JsonFormat: https://github.com/protocolbuffers/protobuf/blob/main/java/util/src/main/java/com/google/protobuf/util/JsonFormat.java#L998

I will try to trick the formatter by overriding default values to the optional fields but even if we did that, oneof fields won't work and it would require some other way of serialization. Just letting you know...

Edit: Imagine something like this:

    final DynamicMessage.Builder defaultInputBuilder = DynamicMessage.newBuilder(method.getInputType());

    method.getInputType().getFields().forEach((field) -> {
        try {
            defaultInputBuilder.setField(field, field.getDefaultValue());
        } catch (Exception e) {
        }
    });

    final DynamicMessage defaultInput = defaultInputBuilder.build();

    final String serialized = defaultExamplePrinter.print(defaultInput).trim();

I added two new fields to test:

  // An optional field
  optional string optional_field = 9;

  // An oneof field
  oneof oneof_field {
    // A possible oneof field
    string first_oneof_field = 10;
    // Another possible oneof field
    string second_oneof_field = 11;
  }

Result before the change:

    {
      "responseType": "COMPRESSABLE",
      "responseSize": 0,
      "fillUsername": false,
      "fillOauthScope": false,
      "responseCompression": "NONE",
    }

Result after the change:

    {
      "responseType": "COMPRESSABLE",
      "responseSize": 0,
      "fillUsername": false,
      "fillOauthScope": false,
      "responseCompression": "NONE",
      "optionalField": "",
      "secondOneofField": ""
    }

Putting all oneof values might require changing the serializer we use (which might be overkill). Or maybe there is another way to generate JSON with all the possible fields that I am unaware of. But I think there is enough win here.

Another thing is that we still do not have a default for message types. The reason why they were disabled was the possibility of recursion. We can just print the default for that message if we haven't recursed yet (will require some additional work).

Dogacel commented 2 years ago

Closing this as we are working on