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

Fix the uri of ArmeriaServerHttpRequest to contain the base url #1904

Merged
merged 2 commits into from Jul 16, 2019

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Jul 15, 2019

Motivation:
The uri of ServerHttpRequest in Spring Web flux contains the base URL.
Also, a negative value of "max-age" from "set-cookie" means no "max-age" attribute, we shouldn't set it.

Modification:

  • Add the base URL when ArmeriaServerHttpRequest is created.
  • Do not set max-age from set-cookie when the value is negative

Result:

  • Spring integration

Motivation:
The uri of `ServerHttpRequest` in Spring Web flux contains the base url.
Also a negative value of "max-age" from "set-cookie" means do "max-age" attribute, we shouldn't set it.

Modification:
- Add the base url when `ArmeriaServerHttpRequest` is created.
- Do not set max-age from set-cookie when the value is negative

Result:
- More Spring integration
@minwoox minwoox added the defect label Jul 15, 2019
@minwoox minwoox added this to the 0.89.0 milestone Jul 15, 2019
@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #1904 into master will decrease coverage by 0.01%.
The diff coverage is 69.23%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1904      +/-   ##
============================================
- Coverage     73.26%   73.25%   -0.02%     
- Complexity     8973     8977       +4     
============================================
  Files           794      794              
  Lines         35162    35170       +8     
  Branches       4317     4322       +5     
============================================
+ Hits          25762    25764       +2     
- Misses         7200     7201       +1     
- Partials       2200     2205       +5
Impacted Files Coverage Δ Complexity Δ
.../spring/web/reactive/ArmeriaServerHttpRequest.java 68.75% <66.66%> (-5.33%) 10 <2> (+2)
...spring/web/reactive/ArmeriaServerHttpResponse.java 75.11% <71.42%> (-0.61%) 32 <0> (+1)
...om/linecorp/armeria/client/HttpSessionHandler.java 59.48% <0%> (-1.73%) 28% <0%> (-1%)
...inecorp/armeria/client/grpc/ArmeriaClientCall.java 80.74% <0%> (-1.49%) 36% <0%> (-1%)
...com/linecorp/armeria/server/HttpServerHandler.java 80.4% <0%> (-0.68%) 79% <0%> (-1%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 87% <0%> (+0.39%) 82% <0%> (+1%) ⬆️
...a/com/linecorp/armeria/client/HttpChannelPool.java 73.54% <0%> (+0.77%) 60% <0%> (+1%) ⬆️
.../java/com/linecorp/armeria/common/HttpRequest.java 75.53% <0%> (+1.06%) 25% <0%> (+1%) ⬆️
.../linecorp/armeria/internal/Http2GoAwayHandler.java 56.41% <0%> (+2.56%) 14% <0%> (+1%) ⬆️

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 40d8367...65a09f4. Read the comment docs.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks!

@trustin
Copy link
Member

trustin commented Jul 16, 2019

Special thanks to @jinsankim for reporting this issue and providing useful hints and sample application. ❤️

@ikhoon ikhoon self-requested a review July 16, 2019 05:56
@trustin trustin merged commit 47624dc into line:master Jul 16, 2019
@minwoox minwoox deleted the fix_ArmeriaServerHttpResponse branch July 16, 2019 08:09
@minwoox
Copy link
Member Author

minwoox commented Jul 16, 2019

Thanks, @trustin and @ikhoon!

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
…#1904)

Motivation:
The uri of `ServerHttpRequest` in Spring Web flux contains the base URL.
Also, a negative value of "max-age" from "set-cookie" means no "max-age" attribute, we shouldn't set it.

Modification:
- Add the base URL when `ArmeriaServerHttpRequest` is created.
- Do not set max-age from set-cookie when the value is negative

Result:
- Spring integration works as expected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants