Support multiple Path annotations #1870

jrhee17 · 2019-06-29T10:54:33Z

Motivation:
A user might want to specify multiple paths for annotated http services.

Modification:

Allow repeatable annotations for @Path annotation.
Different behavior in path declaration (described below)
One AnnotatedHttpService is created for each (httpMethod, pathMapping)
- It is difficult to accommodate multiple pathMappings using a single AnnotatedHttpService because AnnotatedValueResolver may be different
- It may be possible to optimize this by serving multiple httpMethods that have the same pathMapping using a single AnnotatedHttpService at the cost of code complexiy
Merge MethodInfo endpoints with same (httpMethod, methodName)
Known issue: overloaded methods for AnnotatedHttpService are not displayed in docs. I'm thinking this can be tackled in a separate PR.

Breaking change in path declaration behavior

AS-IS

@Get("/")
@Post
public void method1() {} // accepts both Get("/") and Post("/")

TO-BE

@Get("/")
@Post
public void method1() {} // exception thrown. Path declared for Get not applied to Post

trustin · 2019-06-29T13:43:56Z

Test looks good to me. 👍

codecov · 2019-06-29T15:39:02Z

Codecov Report

Merging #1870 into master will increase coverage by 0.01%.
The diff coverage is 97.05%.

@@             Coverage Diff              @@
##             master    #1870      +/-   ##
============================================
+ Coverage     73.59%   73.61%   +0.01%     
- Complexity     9589     9601      +12     
============================================
  Files           838      838              
  Lines         36944    36985      +41     
  Branches       4554     4555       +1     
============================================
+ Hits          27190    27227      +37     
  Misses         7429     7429              
- Partials       2325     2329       +4

Impacted Files	Coverage Δ	Complexity Δ
.../com/linecorp/armeria/server/docs/ServiceInfo.java	`78.94% <100%> (+8.21%)`	`20 <4> (+4)`	⬆️
...ternal/annotation/AnnotatedHttpServiceFactory.java	`87.24% <96.38%> (+1.66%)`	`90 <23> (+10)`	⬆️
...a/com/linecorp/armeria/internal/DefaultValues.java	`66.66% <0%> (-33.34%)`	`4% <0%> (-1%)`
...n/java/com/linecorp/armeria/server/HttpServer.java	`40% <0%> (-20%)`	`2% <0%> (-1%)`
...necorp/armeria/server/GracefulShutdownSupport.java	`97.43% <0%> (-2.57%)`	`4% <0%> (ø)`
.../main/java/com/linecorp/armeria/server/Server.java	`82.74% <0%> (-1.18%)`	`29% <0%> (ø)`
...a/com/linecorp/armeria/common/util/Exceptions.java	`38.59% <0%> (-0.88%)`	`24% <0%> (-1%)`
...com/linecorp/armeria/server/HttpServerHandler.java	`80.73% <0%> (-0.67%)`	`81% <0%> (-1%)`
...inecorp/armeria/server/HttpResponseSubscriber.java	`85.04% <0%> (-0.43%)`	`75% <0%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cca8e3c...84bbda3. Read the comment docs.

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java

trustin · 2019-07-10T12:19:51Z

How's it going? Just let us know if you have any questions working on this. 😉

trustin · 2019-08-08T02:17:27Z

Please let us know once this pull request is ready for reviews.

jrhee17 · 2019-08-09T08:59:25Z

I saw your comment late :P Yep:) I'll remove the WIP label and add a comment when ready

ikhoon · 2019-09-04T01:11:25Z

Fixes: #1822 (just for link)

trustin · 2019-10-04T00:34:01Z

@jrhee17 Thanks a lot for coming back! Is it now ready for reviews?

jrhee17 · 2019-10-04T01:36:01Z

Hi :D . I think overall this is good to go, but let me clean up code/add some documentation/add tests before asking for review. I'll leave a comment when ready

jrhee17 · 2019-10-07T02:58:13Z

I think this PR is (finally) ready for review now~ :-)

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java

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

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactoryTest.java

core/src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceTest.java

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java

...rc/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpDocServicePluginTest.java

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactoryTest.java

core/src/test/java/com/linecorp/armeria/server/docs/ServiceInfoTest.java

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java

trustin

Excellent work, @jrhee17! 👍

minwoox

Left some nits. Thanks a lot @jrhee17!

core/src/test/java/com/linecorp/armeria/server/docs/ServiceInfoTest.java

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java

core/src/test/java/com/linecorp/armeria/server/docs/ServiceInfoTest.java

site/src/sphinx/server-annotated-service.rst

core/src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceTest.java

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

minwoox · 2019-10-18T03:42:42Z

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

@@ -79,14 +79,12 @@ function removeBrackets(headers: string): string {
  return headers.substring(1, length - 1).trim();
 }

-function isExactPathMapping(method: Method): boolean {
+function isSingleExactPathMapping(method: Method): boolean {


We use this value to display different forms in doc service.
If this is true, HttpQueryString shows up. If not, EndpointPath shows up.
When multiple paths are specified, we should display EndpointPath to get a whole path from the user.
So this is the right way to achieve the behavior at the moment. But when we implement this, we should reconsider this behavior. Just left a comment for the remind. 😄

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

minwoox

Thanks a lot, @jrhee17!
You are the best. 👍

ikhoon

Thanks, @jrhee17 This is awesome work 👍👍

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java

minwoox · 2019-10-23T07:48:33Z

Thanks a lot, @jrhee17! 🙇
Hope to see your next work. 😄

jrhee17 requested review from ikhoon, minwoox and trustin as code owners June 29, 2019 10:54

trustin added the new feature label Jun 29, 2019

jrhee17 changed the title ~~[WIP] add unit test for supporting multiple paths~~ [WIP] support multiple @Path annotations Jun 30, 2019

kojilin reviewed Jul 2, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java Outdated Show resolved Hide resolved

jrhee17 force-pushed the feature/multiple-paths branch from 2a7e1b5 to 56dbfb1 Compare August 7, 2019 15:38

jrhee17 force-pushed the feature/multiple-paths branch 4 times, most recently from 8d1f791 to c4e08b7 Compare August 11, 2019 16:33

jrhee17 force-pushed the feature/multiple-paths branch from e93974d to 781d06a Compare October 3, 2019 17:08

jrhee17 force-pushed the feature/multiple-paths branch from 781d06a to d02effe Compare October 6, 2019 14:34

minimal working version of supporting multiple Path annotations

004cb49

jrhee17 force-pushed the feature/multiple-paths branch from d02effe to 004cb49 Compare October 6, 2019 17:59

jrhee17 changed the title ~~[WIP] support multiple @Path annotations~~ Support multiple @Path annotations Oct 7, 2019

trustin requested changes Oct 7, 2019

View reviewed changes

ikhoon reviewed Oct 7, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java Outdated Show resolved Hide resolved

address style related comments, enhance tests

68f12e4

jrhee17 force-pushed the feature/multiple-paths branch from 4814559 to 68f12e4 Compare October 7, 2019 16:27

add failing tests for revised path rules

51e2787

trustin requested changes Oct 10, 2019

View reviewed changes

jrhee17 force-pushed the feature/multiple-paths branch 2 times, most recently from 7e0ea70 to 48d9bed Compare October 10, 2019 16:29

modify path rule to have either http method or path mapping

042691a

jrhee17 force-pushed the feature/multiple-paths branch from 48d9bed to 042691a Compare October 10, 2019 22:34

minwoox reviewed Oct 14, 2019

View reviewed changes

.../src/test/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceBuilderTest.java Show resolved Hide resolved

jrhee17 added 2 commits October 16, 2019 00:21

delegate duplicate path handling to rejectedRouteHandler

396dde0

remove todo statements for checking support for only @path

5d924fc

jrhee17 closed this Oct 17, 2019

jrhee17 reopened this Oct 17, 2019

trustin approved these changes Oct 17, 2019

View reviewed changes

update documentation on path annotation

49bfafd

jrhee17 force-pushed the feature/multiple-paths branch from 4c76bdd to 49bfafd Compare October 17, 2019 22:40

minwoox reviewed Oct 18, 2019

View reviewed changes

fix style issues, add more tests

b8e4f69

minwoox reviewed Oct 21, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/docs/ServiceInfo.java Outdated Show resolved Hide resolved

prefer sorted set over list for serviceinfo methods

2df853e

minwoox approved these changes Oct 22, 2019

View reviewed changes

ikhoon approved these changes Oct 22, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java Outdated Show resolved Hide resolved

predefine decorator that doesn't support Options mapping

84bbda3

jrhee17 force-pushed the feature/multiple-paths branch from 4663ab4 to 84bbda3 Compare October 23, 2019 01:04

minwoox changed the title ~~Support multiple @Path annotations~~ Support multiple Path annotations Oct 23, 2019

minwoox merged commit c760adf into line:master Oct 23, 2019

minwoox added this to the 0.95.0 milestone Oct 23, 2019

minwoox mentioned this pull request Oct 25, 2019

Allow repeating @Path annotations #1822

Closed

jrhee17 deleted the feature/multiple-paths branch August 30, 2021 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple Path annotations #1870

Support multiple Path annotations #1870

jrhee17 commented Jun 29, 2019 •

edited

Loading

trustin commented Jun 29, 2019

codecov bot commented Jun 29, 2019 •

edited

Loading

trustin commented Jul 10, 2019

trustin commented Aug 8, 2019

jrhee17 commented Aug 9, 2019

ikhoon commented Sep 4, 2019

trustin commented Oct 4, 2019

jrhee17 commented Oct 4, 2019

jrhee17 commented Oct 7, 2019

trustin left a comment

minwoox left a comment

minwoox Oct 18, 2019

minwoox left a comment

ikhoon left a comment

minwoox commented Oct 23, 2019

Support multiple Path annotations #1870

Support multiple Path annotations #1870

Conversation

jrhee17 commented Jun 29, 2019 • edited Loading

Breaking change in path declaration behavior

trustin commented Jun 29, 2019

codecov bot commented Jun 29, 2019 • edited Loading

Codecov Report

trustin commented Jul 10, 2019

trustin commented Aug 8, 2019

jrhee17 commented Aug 9, 2019

ikhoon commented Sep 4, 2019

trustin commented Oct 4, 2019

jrhee17 commented Oct 4, 2019

jrhee17 commented Oct 7, 2019

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Oct 18, 2019

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

minwoox commented Oct 23, 2019

jrhee17 commented Jun 29, 2019 •

edited

Loading

codecov bot commented Jun 29, 2019 •

edited

Loading