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

Provide a way to set an example request for AnnotatedServiceRegistrationBean #2026

Conversation

@heowc
Copy link
Contributor

heowc commented Aug 24, 2019

Motivation:

A user cannot specify an example request when adding an annotated service using AnnotatedServiceRegistrationBean.

Modifications:

  • Add AnnotatedExampleRequest for AnnotatedServiceRegistrationBean
  • Add GrpcExampleRequest for GrpcServiceRegistrationBean
  • Remove ExampleRequest
  • Make the AnnotatedExampleRequests are added to DocService

Result:

@heowc heowc requested review from ikhoon, minwoox and trustin as code owners Aug 24, 2019
@ikhoon ikhoon added the new-feature label Aug 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 25, 2019

Codecov Report

Merging #2026 into master will decrease coverage by <.01%.
The diff coverage is 71.73%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2026      +/-   ##
============================================
- Coverage     73.79%   73.78%   -0.01%     
- Complexity     9391     9405      +14     
============================================
  Files           817      819       +2     
  Lines         36121    36161      +40     
  Branches       4452     4453       +1     
============================================
+ Hits          26654    26682      +28     
- Misses         7168     7177       +9     
- Partials       2299     2302       +3
Impacted Files Coverage Δ Complexity Δ
...rp/armeria/spring/GrpcServiceRegistrationBean.java 71.42% <0%> (-28.58%) 3 <0> (ø)
.../web/reactive/ArmeriaReactiveWebServerFactory.java 71.31% <100%> (+0.23%) 19 <0> (ø) ⬇️
...ecorp/armeria/spring/ArmeriaAutoConfiguration.java 80% <100%> (+1.42%) 10 <0> (+2) ⬆️
...eria/internal/spring/ArmeriaConfigurationUtil.java 72% <100%> (+1.05%) 46 <1> (+2) ⬆️
...necorp/armeria/spring/AnnotatedExampleRequest.java 63.63% <63.63%> (ø) 4 <4> (?)
...om/linecorp/armeria/spring/GrpcExampleRequest.java 64.28% <64.28%> (ø) 5 <5> (?)
...meria/spring/AnnotatedServiceRegistrationBean.java 80.76% <66.66%> (-4.24%) 11 <2> (+2)
...meria/common/stream/RegularFixedStreamMessage.java 81.63% <0%> (-6.13%) 13% <0%> (-1%)
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 83.15% <0%> (-3.16%) 13% <0%> (-2%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 86.61% <0%> (-1.58%) 81% <0%> (-1%)
... and 11 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 379d0e8...23ed1cf. Read the comment docs.

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Aug 25, 2019

Thanks for a quick pull request. Let us take a look early next week. Have a happy weekend!

Copy link
Member

minwoox left a comment

Thanks a lot for your work!
I found out that our XServiceRegistrationBean has a problem which can screw up a user while he/she configures the example requests and headers.

  • Cannot set example HTTP headers for gRPC Service.
  • Cannot set example HTTP headers in method level.
  • Cannot set example requests for Thrift in method level.

So I think we should handle this so that a user can configure example requests and headers in service or method levels.
Do you mind handling it in this PR? or I think we can do it in another PR. :-)

@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Aug 30, 2019

Thanks a lot for your work!
I found out that our XServiceRegistrationBean has a problem which can screw up a user while he/she configures the example requests and headers.

  • Cannot set example HTTP headers for gRPC Service.
  • Cannot set example HTTP headers in method level.
  • Cannot set example requests for Thrift in method level.

So I think we should handle this so that a user can configure example requests and headers in service or method levels.
Do you mind handling it in this PR? or I think we can do it in another PR. :-)

I also think it's a good idea to handle the problem in another PR.

@heowc heowc force-pushed the heowc:privode-example-request-for-annotated-service-registration-bean branch from add07a2 to 83c0740 Sep 9, 2019
@heowc heowc closed this Sep 9, 2019
@heowc heowc force-pushed the heowc:privode-example-request-for-annotated-service-registration-bean branch from 83c0740 to 169f509 Sep 9, 2019
@heowc heowc reopened this Sep 10, 2019
@heowc heowc changed the title Provide a way to set an example request for AnnotatedServiceRegistrationBean [WIP] Provide a way to set an example request for AnnotatedServiceRegistrationBean Sep 10, 2019
heowc added 7 commits Sep 10, 2019
…quest-for-annotated-service-registration-bean

# Conflicts:
#	spring/boot-autoconfigure/src/test/java/com/linecorp/armeria/spring/ArmeriaAutoConfigurationTest.java
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Sep 13, 2019

Current exampleRequest is deprecated. As a result, the code is not clean.

Collection<ExampleRequest> exampleRequests = ...

setExampleRequests(...)
getExampleRequests()

...

Collection<GrpcExampleRequest> grpcExampleRequests = ... 

setGrpcExampleRequests(...)
getGrpcExampleRequests()

I need to advice. Do you have a good idea?

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Sep 13, 2019

That's a good question. How about just making a breaking change? i.e. make get/setExampleRequests() use GrpcExampleRequest.

@heowc heowc changed the title [WIP] Provide a way to set an example request for AnnotatedServiceRegistrationBean Provide a way to set an example request for AnnotatedServiceRegistrationBean Sep 13, 2019
heowc added 2 commits Sep 13, 2019
@heowc heowc requested review from trustin, ikhoon and minwoox Sep 13, 2019
heowc added 3 commits Sep 13, 2019
*/
public AnnotatedServiceRegistrationBean setExampleRequests(
@NotNull Collection<AnnotatedExampleRequest> exampleRequests) {
this.exampleRequests = exampleRequests;

This comment has been minimized.

Copy link
@minwoox

minwoox Sep 16, 2019

Member

How about adding exampleRequests to the end of the existing field as we do in HttpHeaders?

This comment has been minimized.

Copy link
@heowc

heowc Sep 16, 2019

Author Contributor

Sorry, I don't understand your comment. 😢 Could you explain more detail?

This comment has been minimized.

Copy link
@minwoox

minwoox Sep 16, 2019

Member

Had a chat with @heowc. 😄

This comment has been minimized.

Copy link
@heowc

heowc Sep 17, 2019

Author Contributor

Thank you for your help at night, @minwoox!!

In summary,
There is generally a consistency that setX is adds to the end of the existing field. However, they are not classes like ~~RegistrationBean. So these classes will go to another PR that needs refactoring.

My English is not good enough, please understand even if you are not good at expression 😢.

heowc added 2 commits Sep 16, 2019
Copy link
Member

minwoox left a comment

Great job! @heowc
Looking forward to seeing your next PR for example headers. 😄

Copy link
Member

trustin left a comment

Great job, @heowc ! 👍

@trustin trustin added this to the 0.92.0 milestone Sep 18, 2019
@ikhoon
ikhoon approved these changes Sep 18, 2019
Copy link
Contributor

ikhoon left a comment

Nice work, thanks! @heowc

@trustin trustin merged commit c87e9cb into line:master Sep 18, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
@heowc heowc deleted the heowc:privode-example-request-for-annotated-service-registration-bean branch Sep 18, 2019
eugene70 pushed a commit to eugene70/armeria that referenced this pull request Oct 16, 2019
…ionBean (line#2026)

Motivation:

A user cannot specify an example request when adding an annotated service using `AnnotatedServiceRegistrationBean`.

Modifications:

- Add `AnnotatedExampleRequest` for `AnnotatedServiceRegistrationBean`
- Add `GrpcExampleRequest` for `GrpcServiceRegistrationBean`
- Remove `ExampleRequest`
- Make the `AnnotatedExampleRequest`s are added to `DocService`

Result:

- Fixes line#1855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.