-
Notifications
You must be signed in to change notification settings - Fork 915
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.
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
Display JSON and protobuf request and response specification in DocService
#4322
Conversation
…ervice` Motivation: A `@RequestObject` bean can be converted as a specification of an annotated service. Neither response nor pojo request are shown in a `DocService`. Only gRPC and Thrift docservice displays the full speficiations of input and output type. This PR aims to generalize a way to create a `StructInfo` from a range of types by offering `NamedTypeInfoProvider` which can be dynamically loaded via SPI or explicitely set using `DocServiceBuilder`. A data type that depend on a specific library, such as protobuf, Thrift, ScalaPB can extend this interface and inject custom implementations. Modifications: - Designed `NamedTypeInfoProvider` interface to let users customize how to create a `StructInfo` from the given type descriptor. The following implmentations are adedd for the default behavior. - `JsonNamedTypeInfoProvider` - `ThriftNamedTypeInfoProvider` - `ProtobufNamedTypeInfoProvider` - `ScalaPbNamedTypeInfoProvider` - Add `DocServiceBuilder.nameTypeInfoProvider()` in order to override the default `NamedTypeInfoProvider`s. - Moved some code related to `StructInfo` in `*DocServicePlugin` to `*NamedTypeInfoProvider` Result: - You can now see the speficication of an annotated service maded of a JSON object or protobuf can be viewed in `DocService`. - Fixes line#4309
Codecov Report
@@ Coverage Diff @@
## master #4322 +/- ##
============================================
+ Coverage 73.93% 74.04% +0.10%
- Complexity 17856 18082 +226
============================================
Files 1516 1527 +11
Lines 66405 67010 +605
Branches 8347 8460 +113
============================================
+ Hits 49099 49620 +521
- Misses 13287 13331 +44
- Partials 4019 4059 +40
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
core/src/main/java/com/linecorp/armeria/server/docs/NamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice overall! Only got to look at the annotated doc part today, will take a closer look tomorrow 🙏
core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. 👍
...src/main/java/com/linecorp/armeria/internal/server/annotation/JsonNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...test/java/com/linecorp/armeria/internal/server/annotation/JsonNamedTypeInfoProviderTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/linecorp/armeria/internal/server/annotation/JsonNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/linecorp/armeria/internal/server/annotation/JsonNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Show resolved
Hide resolved
This PR is ready to review. 😄 |
@cnabro You may be interested in the changes of this PR. |
...test/java/com/linecorp/armeria/internal/server/annotation/JsonNamedTypeInfoProviderTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
Outdated
Show resolved
Hide resolved
Releasing is just around the corner. I rescheduled this features to 1.19.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thanks! 👍 👍 👍
.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java
Show resolved
Hide resolved
.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/internal/server/annotation/ReflectiveNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/internal/server/annotation/ReflectiveNamedTypeInfoProvider.java
Show resolved
Hide resolved
.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/EnumInfo.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/NamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...3/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been waiting for this feature 👍👍
...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java
Show resolved
Hide resolved
...ft0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Show resolved
Hide resolved
...ft0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java
Show resolved
Hide resolved
.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/internal/server/annotation/ReflectiveNamedTypeInfoProvider.java
Show resolved
Hide resolved
.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java
Show resolved
Hide resolved
...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
Outdated
Show resolved
Hide resolved
4c06b48
to
5b3f3f1
Compare
...3/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftNamedTypeInfoProvider.java
Outdated
Show resolved
Hide resolved
...ft0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java
Outdated
Show resolved
Hide resolved
typeDescriptor match { | ||
case clazz: Class[_] if isProtobufMessage(clazz) => | ||
val message = ScalaPbRequestConverterFunction.getDefaultInstance(clazz) | ||
ProtobufNamedTypeInfoProvider.newStructInfo(message.companion.javaDescriptor).withAlias(clazz.getName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q) I'm failing to understand the need for this NamedTypeInfoProvider
😅
As far as I understand, the dependency :scalapb_2.13
implies inclusion of :protobuf
, which means both ScalaPbNamedTypeInfoProvider
and ProtobufNamedTypeInfoProvider
are included as DocService#spiNamedTypeInfoProviders
.
Since it's undefined which NamedTypeInfoProvider
will be applied first (and they seem to be essentially doing the same thing), I was wondering why we define a separate NamedTypeInfoProvider
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScalaPbNamedTypeInfoProvider
extracts protobuf Descriptor
from GeneratedMessage
and GeneratedSealedOneof
using reflections and delegate it to ProtobufNamedTypeInfoProvider
.
If GeneratedMessage
is given to ProtobufNamedTypeInfoProvider
, null
is returned by ProtobufNamedTypeInfoProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If GeneratedMessage is given to ProtobufNamedTypeInfoProvider, null is returned by ProtobufNamedTypeInfoProvider.
Sorry 😅 I wasn't able to understand this point. Would you mind pointing out where GeneratedMessage
yields null
?
Lines 87 to 109 in 5b3f3f1
public NamedTypeInfo newNamedTypeInfo(Object typeDescriptor) { | |
requireNonNull(typeDescriptor, "typeDescriptor"); | |
if (typeDescriptor instanceof Descriptor) { | |
return newStructInfo((Descriptor) typeDescriptor); | |
} | |
if (typeDescriptor instanceof EnumDescriptor) { | |
return newEnumInfo((EnumDescriptor) typeDescriptor); | |
} | |
if (!(typeDescriptor instanceof Class)) { | |
return null; | |
} | |
final Class<?> clazz = (Class<?>) typeDescriptor; | |
if (!isProtobufMessage(clazz)) { | |
return null; | |
} | |
final Message.Builder messageBuilder = getMessageBuilder(clazz); | |
final Descriptor descriptorForType = messageBuilder.getDescriptorForType(); | |
return newStructInfo(descriptorForType).withAlias(clazz.getName()); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scalapb.GeneratedMessage
is not a subclass of com.google.protobuf.Message
so isProtobufMessage()
returns false for scalapb.GeneratedMessage
.
Lines 102 to 104 in 11ee1ff
if (!isProtobufMessage(clazz)) { | |
return null; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some tests 61dd8c0
(#4322) in order to check your concerns. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People who use DocServic as the API spec will love this feature. Thanks! 😄
@ikhoon Could you fix the conflict? 😄 |
Fixed. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ikhoon ! 👍 🙇 👍
…ervice` (line#4322) Motivation: A `@RequestObject` bean can be converted as a specification of an annotated service. Neither response nor POJO request are shown in a `DocService`. Only gRPC and Thrift `DocService` can display the full specifications of input and output types. This PR aims to generalize a way to create a `StructInfo` from a range of types by offering `NamedTypeInfoProvider` which can be dynamically loaded via SPI or explicitly set using `DocServiceBuilder`. A data type that depends on a specific library, such as protobuf, Thrift, ScalaPB can extend this interface and inject custom implementations. Modifications: - Designed `NamedTypeInfoProvider` interface to let users customize how to create a `StructInfo` from the given type descriptor. The following implementations are added for the default behavior. - `JsonNamedTypeInfoProvider` - `ThriftNamedTypeInfoProvider` - `ProtobufNamedTypeInfoProvider` - `ScalaPbNamedTypeInfoProvider` - Add `DocServiceBuilder.nameTypeInfoProvider()` in order to override the default `NamedTypeInfoProvider`s. - Moved some code related to `StructInfo` in `*DocServicePlugin` to `*NamedTypeInfoProvider` Result: - You can now see the specification of an annotated service made of a JSON object or protobuf can be viewed in `DocService`. - Fixes line#4309
Motivation:
A
@RequestObject
bean can be converted as a specification of anannotated service. Neither response nor POJO request are shown in a
DocService
. Only gRPC and ThriftDocService
can display the fullspecifications of input and output types.
This PR aims to generalize a way to create a
StructInfo
from a rangeof types by offering
NamedTypeInfoProvider
which can be dynamicallyloaded via SPI or explicitly set using
DocServiceBuilder
.A data type that depends on a specific library, such as protobuf,
Thrift, ScalaPB can extend this interface and inject custom
implementations.
Modifications:
NamedTypeInfoProvider
interface to let users customize how tocreate a
StructInfo
from the given type descriptor.The following implementations are added for the default behavior.
JsonNamedTypeInfoProvider
ThriftNamedTypeInfoProvider
ProtobufNamedTypeInfoProvider
ScalaPbNamedTypeInfoProvider
DocServiceBuilder.nameTypeInfoProvider()
in order to overridethe default
NamedTypeInfoProvider
s.StructInfo
in*DocServicePlugin
to*NamedTypeInfoProvider
Result:
object or protobuf can be viewed in
DocService
.StructInfo
for various types #4309