Skip to content
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

Merged
merged 27 commits into from Sep 13, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jun 28, 2022

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 NamedTypeInfoProviders.
  • Moved some code related to StructInfo in *DocServicePlugin to
    *NamedTypeInfoProvider

Result:

…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
@ikhoon ikhoon added this to the 1.18.0 milestone Jun 28, 2022
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #4322 (53475b1) into master (3da7bac) will increase coverage by 0.10%.
The diff coverage is 83.70%.

@@             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     
Impacted Files Coverage Δ
...linecorp/armeria/server/docs/DocServicePlugin.java 20.00% <ø> (ø)
...om/linecorp/armeria/server/docs/NamedTypeInfo.java 100.00% <ø> (ø)
...orp/armeria/server/docs/NamedTypeInfoProvider.java 0.00% <0.00%> (ø)
...ver/protobuf/ProtobufRequestConverterFunction.java 77.52% <ø> (ø)
...obuf/ProtobufRequestConverterFunctionProvider.java 92.85% <ø> (+25.00%) ⬆️
...ver/annotated/kotlin/MarkdownDescriptionService.kt 29.41% <25.00%> (ø)
...inecorp/armeria/server/docs/DocServiceBuilder.java 73.91% <36.36%> (-2.91%) ⬇️
...om/linecorp/armeria/server/docs/ExceptionInfo.java 57.14% <45.00%> (-9.53%) ⬇️
...ava/com/linecorp/armeria/server/docs/EnumInfo.java 44.18% <45.83%> (+6.25%) ⬆️
...armeria/internal/server/annotation/KotlinUtil.java 71.11% <50.00%> (-1.51%) ⬇️
... and 86 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@jrhee17 jrhee17 left a 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 🙏

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. 👍

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 1, 2022

This PR is ready to review. 😄

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 1, 2022

@cnabro You may be interested in the changes of this PR.

@ikhoon ikhoon modified the milestones: 1.18.0, 1.19.0 Aug 3, 2022
@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 3, 2022

Releasing is just around the corner. I rescheduled this features to 1.19.0.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Thanks! 👍 👍 👍

Copy link
Contributor

@cnabro cnabro left a 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 👍👍

typeDescriptor match {
case clazz: Class[_] if isProtobufMessage(clazz) =>
val message = ScalaPbRequestConverterFunction.getDefaultInstance(clazz)
ProtobufNamedTypeInfoProvider.newStructInfo(message.companion.javaDescriptor).withAlias(clazz.getName())
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

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());
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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. :-)

Copy link
Member

@minwoox minwoox left a 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! 😄

@minwoox
Copy link
Member

minwoox commented Sep 7, 2022

@ikhoon Could you fix the conflict? 😄

@ikhoon
Copy link
Contributor Author

ikhoon commented Sep 8, 2022

Fixed. 😉

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ikhoon ! 👍 🙇 👍

@ikhoon ikhoon merged commit 755c282 into line:master Sep 13, 2022
@ikhoon ikhoon deleted the struct-info-provider branch September 13, 2022 01:48
heowc pushed a commit to heowc/armeria that referenced this pull request Sep 24, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to support StructInfo for various types
4 participants