Make server build fail when duplicate route and fail handler.#6499
Make server build fail when duplicate route and fail handler.#6499
Conversation
chickenchickenlove
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6499 +/- ##
============================================
- Coverage 74.46% 74.16% -0.30%
- Complexity 22234 23028 +794
============================================
Files 1963 2064 +101
Lines 82437 86262 +3825
Branches 10764 11313 +549
============================================
+ Hits 61385 63979 +2594
- Misses 15918 16882 +964
- Partials 5134 5401 +267 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } catch (IllegalStateException shouldFailException) { | ||
| throw shouldFailException; | ||
| } catch (Exception e) { | ||
| logger.warn("Unexpected exception from a {}:", | ||
| RejectedRouteHandler.class.getSimpleName(), e); |
There was a problem hiding this comment.
I personally prefer removing these catch clauses and changing the signature of handleDuplicateRoute. But since that was a breaking change, I'm fine with the current change. Thanks!
There was a problem hiding this comment.
What do you think of introducing a new exception (e.g. DuplicateRouteException) instead of using IllegalStateException? This might prevent rethrowing an IllegalStateException which is raised from a user-defined handler.
There was a problem hiding this comment.
@minwoox
Sounds good. I prefer to do so as well!
Let me make new commits based on your comments! 🙇♂️
| import com.linecorp.armeria.client.DuplicateRouteException; | ||
| import com.linecorp.armeria.common.HttpResponse; | ||
|
|
||
| public class RejectRouterHandlerTest { |
There was a problem hiding this comment.
| public class RejectRouterHandlerTest { | |
| class RejectRouterHandlerTest { |
ikhoon
left a comment
ikhoon
left a comment
There was a problem hiding this comment.
Thanks, @chickenchickenlove 👍👍
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
Still, looks great. Thanks!
4 participants
Motivation:
RejectRouterHandler.FAILdoes not work as intended.Modifications:
rejectionHandler.handleDuplicateRoute(virtualHost, route, existingRoute)catchIllegalStateExceptionand throw it.ContextPathTest. (ContextPathTestalready has duplicated route withRejectRouterHandler.FAIL, but it just warn not fail. )Result:
RejectRouterHandler.FAILis configured,Server build will be failed.
RejectRouterHandler.FAILdoes not work as intended. #6498