Fix that overloaded methods are not displayed in DocService #4545

minwoox · 2022-11-21T08:37:36Z

Motivation:
Overloaded methods in annotated services are not displayed correctly in DocService.
Armeria DocService was built for Thrift services in which overloading methods are not possible, so we didn't consider overloaded methods at that time.
Also, it was for Thrift types, the TypeSignature wasn't structured well to represent other types used in gRPC and annotated services.

Modifications:

Add id to MethodInfo that uniquely identifies each method.
- It is formed of serviceName/methodName/httpMethod
- The methodName has increased -n at the end when the method is overloaded.
Fix a bug where parsing the same named type recursively.
(Breaking)
- Interface TypeSigniture.
  - Add subclasses with TypeSignatureType that represents each TypeSignature
  - NamedTypeInfo.findNamedTypes() now returns Set<NamedTypeSignature>.

Result:

You can now send a debug request via DocService when the method is overloaded.
Well-structured TypeSignature

To-do:

Add JsonSchemaType to TypeSignature for creating the JSON schema easily.

Dogacel

Does front-end need to know about overloadId?

Dogacel · 2022-11-21T09:12:46Z

...3/src/test/scala/com/linecorp/armeria/server/protobuf/ScalaPbNamedTypeInfoProviderTest.scala

+    println(json5Mapper.writerWithDefaultPrettyPrinter().writeValueAsString(response.get("services").get(0).get("methods")))
+
+


Seems like a leftover

Dogacel · 2022-11-21T09:16:21Z

docs-client/src/containers/MethodPage/index.tsx

-      m.httpMethod === props.match.params.httpMethod,
-  );
+  const id = `${params.serviceName}/${params.methodName}/${params.httpMethod}`;
+  const method = service.methods.find((m) => m.id === id);


👍 Great to see this change, did we have issues with overlapping method names from different services in the previous version?

overlapping method names from different services in the previous version?

No, we didn't have such an issue. We have issues only with the same service. For example:

class MyAnnotatedService { @Get("/foo) public HttpResponse foo() {...} @Get("/foo1) public HttpResponse foo(MyType type) {...} <--- This isn't rendered. } class YourAnnotatedService { @Get("/foo2) public HttpResponse foo() {...} // Rendered correctly }

Dogacel · 2022-11-21T09:18:19Z

core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java

@@ -354,7 +373,7 @@ private static void addMarkdownDescriptionMethodInfo(Map<Class<?>, Set<MethodInf
                         .descriptionInfo(DescriptionInfo.of("DESCRIPTION `PARAM`", Markup.MARKDOWN))
                         .build());
        final MethodInfo methodInfo = new MethodInfo(
-                "description", STRING, fieldInfos, ImmutableList.of(),
+                MyService.class.getName(), "description", 0, STRING, fieldInfos, ImmutableList.of(),


just double-checking, does this include the package name?

Yes and let me show later how we find the corresponding struct in StructInfo or enum in EnumInfo later. 😉

Dogacel · 2022-11-21T09:20:14Z

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignatureType.java

+    /**
+     * Container type.
+     */
+    CONTAINER(true, false),


What is an example for a container type? Maybe generics? Like Id<T> ?

One example would be:

armeria/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java

Line 518 in 6bd02dc

public BiFunction<JsonNode, ?, String> consumes() {

Of course, we cannot represent this type with JSON schema so I'm thinking we just need to omit such fields
and add "additionalProperties": "true"

If it is something simple like public class Box<T> { public T shape; ... } I guess FieldInfo will already contain what we have put inside T right? If not, the user might need to find out that from additional properties (which seems mostly redundant, I agree with you.)

Dogacel · 2022-11-21T09:24:18Z

...es/com/linecorp/armeria/server/protobuf/ScalaPbNamedTypeInfoProviderTest_specification.json5

@@ -254,41 +255,7 @@
                          "location": "UNSPECIFIED",
                          "requirement": "OPTIONAL",
                          "typeSignature": "armeria.protobuf.testing.SimpleOneof",
-                          "childFieldInfos": [


Just double checking, are we removing childFieldInfos from the return type signatures for all types? What is this about?

It's a bug. There's a recursion so I removed it. You can find armeria.protobuf.testing.SimpleOneof" in two levels above.

codecov · 2022-11-21T09:56:23Z

Codecov Report

Base: 74.08% // Head: 74.05% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (ce97d4c) compared to base (b127cd2).
Patch coverage: 89.26% of modified lines in pull request are covered.

❗ Current head ce97d4c differs from pull request most recent head 63cc314. Consider uploading reports for the commit 63cc314 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4545      +/-   ##
============================================
- Coverage     74.08%   74.05%   -0.03%     
+ Complexity    18190    18186       -4     
============================================
  Files          1537     1542       +5     
  Lines         67432    67530      +98     
  Branches       8533     8549      +16     
============================================
+ Hits          49959    50012      +53     
- Misses        13401    13438      +37     
- Partials       4072     4080       +8

Impacted Files	Coverage Δ
...er/annotation/ReflectiveNamedTypeInfoProvider.java	`88.46% <0.00%> (ø)`
...om/linecorp/armeria/server/docs/NamedTypeInfo.java	`0.00% <ø> (-100.00%)`	⬇️
...rp/armeria/server/docs/ContainerTypeSignature.java	`62.50% <62.50%> (ø)`
...a/internal/server/annotation/AnnotatedService.java	`87.89% <66.66%> (-0.29%)`	⬇️
...necorp/armeria/server/docs/NamedTypeSignature.java	`76.19% <76.19%> (ø)`
...a/com/linecorp/armeria/server/docs/MethodInfo.java	`74.72% <83.33%> (+0.97%)`	⬆️
...corp/armeria/server/docs/DefaultTypeSignature.java	`84.21% <84.21%> (ø)`
...erver/annotation/DefaultNamedTypeInfoProvider.java	`92.71% <87.50%> (-0.39%)`	⬇️
...a/com/linecorp/armeria/server/docs/DocService.java	`90.66% <89.01%> (-3.14%)`	⬇️
...server/protobuf/ProtobufNamedTypeInfoProvider.java	`89.00% <91.66%> (+0.11%)`	⬆️
... and 43 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

minwoox · 2022-11-21T13:22:33Z

Does front-end need to know about overloadId?

Nope. MethodInfo.id() has the overloadId in it and it's all the front-end needs. 😄

Dogacel · 2022-11-21T14:24:00Z

docs-client/src/containers/MethodPage/index.tsx

-      m.name === props.match.params.methodName &&
-      m.httpMethod === props.match.params.httpMethod,
-  );
+  const id = `${params.serviceName}/${params.methodName}/${params.httpMethod}`;


Should we include overloadId here as well then?

No, we shouldn't. 😉
params.methodName is the path parameter that is inserted from the Router.

armeria/docs-client/src/containers/App/index.tsx

Line 391 in 6bd02dc

path="/methods/:serviceName/:methodName/:httpMethod"

It could be overload-1 which already has the overloadId if a user types or navigates through the link.

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignatureType.java

core/src/main/java/com/linecorp/armeria/server/docs/ContainerTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/DefaultTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/NamedTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignatureType.java

core/src/main/java/com/linecorp/armeria/server/docs/MethodInfo.java

core/src/main/java/com/linecorp/armeria/server/docs/NamedTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/DefaultTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignature.java

.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java

minwoox

@ikhoon Addressed your comments so PTAL.
Thanks a lot for the detailed review. 😉

core/src/main/java/com/linecorp/armeria/server/docs/DefaultTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/MethodInfo.java

core/src/main/java/com/linecorp/armeria/server/docs/NamedTypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignature.java

core/src/main/java/com/linecorp/armeria/server/docs/TypeSignatureType.java

grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java

.../main/java/com/linecorp/armeria/internal/server/annotation/DefaultNamedTypeInfoProvider.java

ikhoon

Thanks, @minwoox and @Dogacel! 🚀🚀

ikhoon · 2022-12-06T03:05:05Z

...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java

@@ -412,7 +419,7 @@ static TypeSignature toTypeSignature(Type type) {
            return TypeSignature.ofList(toTypeSignature(clazz.getComponentType()));
        }

-        return TypeSignature.ofNamed(clazz);
+        return TypeSignature.ofStruct(clazz);


ikhoon · 2022-12-06T03:18:25Z

core/src/main/java/com/linecorp/armeria/server/docs/ContainerTypeSignature.java

+
+    @Override
+    public String signature() {
+        return name() + '<' + Joiner.on(", ").join(typeParameters) + '>';


Joiner.on(...) creates a new object. Should we use a static variable?
https://github.com/google/guava/wiki/StringsExplained#joiner

Copied the previous logic without much thinking. 😅 Thanks!

ikhoon · 2022-12-06T03:23:14Z

core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java

-        requireNonNull(namedTypeInfoProviders, "namedTypeInfoProviders");
-        for (NamedTypeInfoProvider typeInfoProvider : namedTypeInfoProviders) {
-            namedTypeInfoProvider(typeInfoProvider);
+    public DocServiceBuilder descriptiveTypeInfoProvider(


It seems like I missed adding @UnstableApi. 😅 Could you add it to this method and below?

Oops, added. 😉

ikhoon · 2022-12-06T03:31:23Z

core/src/main/java/com/linecorp/armeria/server/docs/MethodInfo.java

@@ -169,7 +209,7 @@ public MethodInfo withParameters(Iterable<FieldInfo> parameters) {

        return new MethodInfo(name, returnTypeSignature, parameters, exceptionTypeSignatures, endpoints,
                              exampleHeaders, exampleRequests, examplePaths, exampleQueries, httpMethod,
-                              descriptionInfo);
+                              descriptionInfo, id);


Oops... I found the Javadoc is incorrect. Could you fix it as well? 😅

minwoox · 2022-12-06T09:07:11Z

Let me merge this so I can continue to add JSON schema type for @Dogacel's work.
Thanks for reviewing. 😉

Dogacel · 2022-12-06T10:51:43Z

@minwoox Thanks a lot 🙇 I haven't been able to look at armeria for the last couple of weeks because this PR was waiting on open status. Are there any help needed? I expect the changes to be trivial, mostly enum logic will change.

minwoox · 2022-12-06T11:11:00Z

Are there any help needed? I expect the changes to be trivial, mostly enum logic will change.

Yes, I need your help. 😄
I am planning to send the follow-up PR that adds JSON schema type and adds references for look-up. Please take a look at it and give feedback if there's anything I need to make change for your #4518.

WIP

6bd02dc

minwoox added defect new feature breaking change labels Nov 21, 2022

minwoox requested review from ikhoon, jrhee17 and trustin as code owners November 21, 2022 08:37

Add unstable api

ce97d4c

minwoox mentioned this pull request Nov 21, 2022

Generate JSON Schema from ServiceSpecifications #4518

Merged

Dogacel reviewed Nov 21, 2022

View reviewed changes

Fix test

f5598ee

minwoox added 2 commits November 21, 2022 22:27

Fix Scala lint

c05ee4f

Add imports

c4bebeb

Dogacel reviewed Nov 21, 2022

View reviewed changes

ikhoon reviewed Dec 3, 2022

View reviewed changes

Address comments by @ikhoon

5f94f33

minwoox commented Dec 5, 2022

View reviewed changes

minwoox added 2 commits December 5, 2022 17:38

Fix

239d9a8

Lint

4de3379

ikhoon approved these changes Dec 6, 2022

View reviewed changes

Address comments by @ikhoon

63cc314

minwoox merged commit ef64fde into line:master Dec 6, 2022

minwoox deleted the fix_docservice_overload branch December 6, 2022 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that overloaded methods are not displayed in DocService #4545

Fix that overloaded methods are not displayed in DocService #4545

minwoox commented Nov 21, 2022 •

edited

Loading

Dogacel left a comment

Dogacel Nov 21, 2022

Dogacel Nov 21, 2022

minwoox Nov 21, 2022 •

edited

Loading

Dogacel Nov 21, 2022

minwoox Nov 21, 2022

Dogacel Nov 21, 2022

minwoox Nov 21, 2022 •

edited

Loading

Dogacel Nov 21, 2022

Dogacel Nov 21, 2022

minwoox Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

minwoox commented Nov 21, 2022

Dogacel Nov 21, 2022

minwoox Nov 21, 2022 •

edited

Loading

minwoox left a comment

ikhoon left a comment

ikhoon Dec 6, 2022

ikhoon Dec 6, 2022

minwoox Dec 6, 2022

ikhoon Dec 6, 2022

minwoox Dec 6, 2022

ikhoon Dec 6, 2022

minwoox commented Dec 6, 2022

Dogacel commented Dec 6, 2022

minwoox commented Dec 6, 2022

		println(json5Mapper.writerWithDefaultPrettyPrinter().writeValueAsString(response.get("services").get(0).get("methods")))

Fix that overloaded methods are not displayed in DocService #4545

Fix that overloaded methods are not displayed in DocService #4545

Conversation

minwoox commented Nov 21, 2022 • edited Loading

Dogacel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Nov 21, 2022 • edited Loading

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

minwoox Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2022 • edited Loading

Codecov Report

minwoox commented Nov 21, 2022

Choose a reason for hiding this comment

minwoox Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon 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

minwoox commented Dec 6, 2022

Dogacel commented Dec 6, 2022

minwoox commented Dec 6, 2022

minwoox commented Nov 21, 2022 •

edited

Loading

minwoox Nov 21, 2022 •

edited

Loading

minwoox Nov 21, 2022 •

edited

Loading

minwoox Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

minwoox Nov 21, 2022 •

edited

Loading