Implement @Attribute Injection. #5547

chickenchickenlove · 2024-03-29T08:36:43Z

Motivation:

In the past, users could get a value from ServiceRequestContext by using ServiceRequestContext.attr(key). however, if it were possible to inject values which in ServiceRequestContext into method parameters using any annotation, users would be able to use it more conveniently.

Modifications:

New annotation. @Attribute is added.
Make new field rawType in AnnotatedValueResolver to validate type cast.
New methods ofAttribute() and attributeResolver() are added to AnnotatedValueResolver to get values from ServiceRequestContext and inject them method parameters.
findName() in AnnotatedElementNameUtil method have been merged into a single method. (refactoring for internal use)
Add test codes.
Add document explaining how to use the @Attribute annotation.

Result:

Closes Support for @Attribute in annotated services #5514
Users can inject values from the ServiceRequestContext into method parameters using the @Attribute annotation without ServiceRequestContext.attr(key).

CLAassistant · 2024-03-29T08:36:50Z

All committers have signed the CLA.

github-actions · 2024-03-29T08:53:40Z

🔍 Build Scan® (commit: `70d76cb`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/lu7vfihw5qooe
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/bockuhidf4g3s
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/4qbeilnrztoqq
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/jv2vv4gi2bf34
build-self-hosted-unsafe-jdk-17-min-java-11	❌ (failure)	https://ge.armeria.dev/s/bdumqrgjh34h4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/fo7wwfesfgic6
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/3enumq5mr3v7i
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/m5xpss23od44i

codecov · 2024-03-29T09:25:56Z

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 74.11%. Comparing base (b8eb810) to head (1d0b6c7).
Report is 190 commits behind head on main.

❗ Current head 1d0b6c7 differs from pull request most recent head 70d76cb. Consider uploading reports for the commit 70d76cb to get more accurate results

Files	Patch %	Lines
...rnal/server/annotation/AnnotatedValueResolver.java	90.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5547      +/-   ##
============================================
+ Coverage     73.95%   74.11%   +0.16%     
- Complexity    20115    21019     +904     
============================================
  Files          1730     1819      +89     
  Lines         74161    77418    +3257     
  Branches       9465     9891     +426     
============================================
+ Hits          54847    57380    +2533     
- Misses        14837    15385     +548     
- Partials       4477     4653     +176

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trustin

Good start! Looking forward to the update 🙇

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

core/src/main/java/com/linecorp/armeria/server/annotation/Attribute.java

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

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

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

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

trustin

Could you fix all lint errors?

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

core/src/main/java/com/linecorp/armeria/server/annotation/Attribute.java

...rc/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolverTest.java

chickenchickenlove · 2024-04-23T07:58:54Z

Re-requesting review sounds like a bug.
https://github.com/line/armeria/actions/runs/8796330970/job/24138993146?pr=5547#step:2:10
It seems that component-owners plugin takes the case into account though.
https://github.com/dyladan/component-owners/blob/cdaadffde64c918909ee081e3fe044b8910f56c2/src/main.ts#L64-L65

@ikhoon nim, Thanks for your comment.
By the way, i lost your approve again 😢... after commit suggestion from @minwoox nim.

@ikhoon nim, @minwoox nim, sorry to bother you.
I lost you guys approve 😢
when you have free time, please take a look again 🙇‍♂️...

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

trustin

I think we're pretty close again, now. Thanks a lot for your patience, @chickenchickenlove! 🙇

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

...rc/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolverTest.java

core/src/main/java/com/linecorp/armeria/server/annotation/Attribute.java

…tion/AnnotatedValueResolver.java Co-authored-by: Trustin Lee <t@motd.kr>

…tion/AnnotatedValueResolverTest.java Co-authored-by: Trustin Lee <t@motd.kr>

ikhoon

Still looks good! 👍

ikhoon · 2024-05-02T08:21:16Z

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

+            if (targetType.isPrimitive()) {
+                targetType = Primitives.wrap(targetType);
+            }


Micro optimization) Should we do this when rawType is created to avoid additional costs?

You are right 👍

Using rawType might result in some additional costs due to Wrapping in all AnnotatedValueResolvers that do not use rawType. However, unnecessary calls will be made at most once during initialization, and afterwards, it could save on the cost of Wrapping with each incoming user request.

core/src/main/java/com/linecorp/armeria/server/annotation/Attribute.java

…ibute.java Co-authored-by: Ikhun Um <ih.pert@gmail.com>

trustin

trustin · 2024-05-07T13:42:53Z

Oops! Sorry but would you mind resolving the merge conflict, @chickenchickenlove ?

chickenchickenlove · 2024-05-07T14:00:41Z

@trustin nim, thanks for you to notify me 🙇‍♂️
I made new commit to resolve conflict.
I guess this PR cause the conflict. 🤔
I updated the PR description as well.

trustin · 2024-05-08T05:32:27Z

Thanks a lot for updating the PR description and resolving the merge conflict, @chickenchickenlove! 🙇🙇🙇

@jrhee17 @minwoox PTAL

minwoox

Still LGTM. Thanks!

jrhee17

👍 👍 👍

### Motivation: - In the past, users could get a value from `ServiceRequestContext` by using `ServiceRequestContext.attr(key)`. however, if it were possible to inject values which in `ServiceRequestContext` into method parameters using any annotation, users would be able to use it more conveniently. ### Modifications: - New annotation. `@Attribute` is added. - Make new field `rawType` in `AnnotatedValueResolver` to validate type cast. - New methods `ofAttribute()` and `attributeResolver()` are added to `AnnotatedValueResolver` to get values from `ServiceRequestContext` and inject them method parameters. - `findName()` in `AnnotatedElementNameUtil` method have been merged into a single method. (refactoring for internal use) - Add test codes. - Add document explaining how to use the `@Attribute` annotation. ### Result: - Closes line#5514 - Users can inject values from the `ServiceRequestContext` into method parameters using the `@Attribute` annotation without `ServiceRequestContext.attr(key)`. --------- Co-authored-by: Trustin Lee <t@motd.kr> Co-authored-by: Ikhun Um <ih.pert@gmail.com> Co-authored-by: minux <songmw725@gmail.com>

Motivation: We must not convert the header name specified in `@Header` when creating an annotated service. This is a regression introduced by #5547, affecting 1.29.0 through 1.29.3. Modifications: - Do not convert the header name specified in `@Header`. Result: - An annotated service that has a `@Header` with the specified name now works correctly.

Motivation: We must not convert the header name specified in `@Header` when creating an annotated service. This is a regression introduced by line#5547, affecting 1.29.0 through 1.29.3. Modifications: - Do not convert the header name specified in `@Header`. Result: - An annotated service that has a `@Header` with the specified name now works correctly.

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin March 29, 2024 08:36

skeleton code for @Attribute injection.

eac6431

chickenchickenlove force-pushed the 240329-try1 branch from 28bc91d to eac6431 Compare March 29, 2024 08:48

trustin reviewed Apr 1, 2024

View reviewed changes

Implement Attribute Injection : Apply review

2801cdd

chickenchickenlove force-pushed the 240329-try1 branch from 52f3f53 to 2801cdd Compare April 1, 2024 11:28

make test code for Attribute injection.

889d887

chickenchickenlove force-pushed the 240329-try1 branch from 3e809c7 to 889d887 Compare April 1, 2024 13:11

chickenchickenlove requested a review from trustin April 1, 2024 13:33

remove duplicate code from AnnotatedValueResolverTest

94a93af

trustin reviewed Apr 2, 2024

View reviewed changes

Implement Attribute Injection : Apply review2

42ef9a2

chickenchickenlove force-pushed the 240329-try1 branch from 126284b to 42ef9a2 Compare April 2, 2024 07:09

chickenchickenlove requested a review from trustin April 2, 2024 07:11

trustin reviewed Apr 2, 2024

View reviewed changes

trustin added new feature sprint Issues for OSS Sprint participants labels Apr 2, 2024

Implement Attribute Injection : Apply review3

ab8a08a

chickenchickenlove requested a review from trustin April 3, 2024 01:54

Implement Attribute Injection : Apply review3. remove useless comments.

3fac84e

trustin reviewed Apr 3, 2024

View reviewed changes

Fix lint, Apply review

f3f8b74

chickenchickenlove requested a review from trustin April 3, 2024 11:04

Add user docs

821e18f

minwoox approved these changes Apr 23, 2024

View reviewed changes

trustin reviewed Apr 23, 2024

View reviewed changes

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

trustin self-requested a review April 23, 2024 14:07

throw immedetely if valued is failed to be cast.

65bc886

github-actions bot requested a review from minwoox April 23, 2024 23:00

trustin reviewed Apr 24, 2024

View reviewed changes

trustin self-requested a review April 24, 2024 02:24

chickenchickenlove and others added 5 commits April 24, 2024 11:59

Update core/src/main/java/com/linecorp/armeria/internal/server/annota…

c27629b

…tion/AnnotatedValueResolver.java Co-authored-by: Trustin Lee <t@motd.kr>

Update core/src/main/java/com/linecorp/armeria/internal/server/annota…

3943f70

…tion/AnnotatedValueResolver.java Co-authored-by: Trustin Lee <t@motd.kr>

Update core/src/test/java/com/linecorp/armeria/internal/server/annota…

3df348b

…tion/AnnotatedValueResolverTest.java Co-authored-by: Trustin Lee <t@motd.kr>

apply review

7230534

fix lint error.

72d26b2

ikhoon added this to the 1.29.0 milestone May 2, 2024

ikhoon approved these changes May 2, 2024

View reviewed changes

chickenchickenlove and others added 2 commits May 2, 2024 20:26

Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…

8f3631c

…ibute.java Co-authored-by: Ikhun Um <ih.pert@gmail.com>

apply review

9013388

trustin approved these changes May 2, 2024

View reviewed changes

Merge branch 'main' into 240329-try1

588c8bb

fix lint error

70d76cb

minwoox approved these changes May 8, 2024

View reviewed changes

jrhee17 approved these changes May 8, 2024

View reviewed changes

trustin merged commit 727ac80 into line:main May 9, 2024
14 of 15 checks passed

minwoox mentioned this pull request Jul 29, 2024

Do not convert @Header name when the name is specified #5841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement @Attribute Injection. #5547

Implement @Attribute Injection. #5547

chickenchickenlove commented Mar 29, 2024 •

edited

Loading

CLAassistant commented Mar 29, 2024 •

edited

Loading

github-actions bot commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

trustin left a comment

trustin left a comment

chickenchickenlove commented Apr 23, 2024

trustin left a comment

ikhoon left a comment

ikhoon May 2, 2024

chickenchickenlove May 2, 2024

trustin left a comment

trustin commented May 7, 2024

chickenchickenlove commented May 7, 2024 •

edited

Loading

trustin commented May 8, 2024

minwoox left a comment

jrhee17 left a comment

Implement @Attribute Injection. #5547

Implement @Attribute Injection. #5547

Conversation

chickenchickenlove commented Mar 29, 2024 • edited Loading

Motivation:

Modifications:

Result:

CLAassistant commented Mar 29, 2024 • edited Loading

github-actions bot commented Mar 29, 2024 • edited Loading

🔍 Build Scan® (commit: 70d76cb)

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

chickenchickenlove commented Apr 23, 2024

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon May 2, 2024

Choose a reason for hiding this comment

chickenchickenlove May 2, 2024

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin commented May 7, 2024

chickenchickenlove commented May 7, 2024 • edited Loading

trustin commented May 8, 2024

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

chickenchickenlove commented Mar 29, 2024 •

edited

Loading

CLAassistant commented Mar 29, 2024 •

edited

Loading

github-actions bot commented Mar 29, 2024 •

edited

Loading

🔍 Build Scan® (commit: `70d76cb`)

codecov bot commented Mar 29, 2024 •

edited

Loading

chickenchickenlove commented May 7, 2024 •

edited

Loading