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.31%
- Complexity 22234 23027 +793
============================================
Files 1963 2064 +101
Lines 82437 86262 +3825
Branches 10764 11313 +549
============================================
+ Hits 61385 63974 +2589
- Misses 15918 16885 +967
- Partials 5134 5403 +269 ☔ 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 { |
| * <p>This exception is raised when multiple routes share an identical path | ||
| * and thus cannot be uniquely distinguished.</p> | ||
| */ | ||
| public final class DuplicateRouteException extends RuntimeException { |
There was a problem hiding this comment.
Do you think we should place this in the server-side package?
| * <p>This exception is raised when multiple routes share an identical path | ||
| * and thus cannot be uniquely distinguished.</p> | ||
| */ | ||
| public final class DuplicateRouteException extends RuntimeException { |
There was a problem hiding this comment.
| public final class DuplicateRouteException extends RuntimeException { | |
| @UnstableApi | |
| public final class DuplicateRouteException extends RuntimeException { |
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