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 not to raise an exception from `(Service|Client)RequestContext.cu… #2962
Conversation
…rrentOrNull()` Motivation: `ServiceRequestContext.currentOrNull()` raises an `IllegalArgumentException` if the current ctx is `ClientRequestContext` and `ctx.root()` returns `null`. It's very confusing because all other APIs returns `null` if the name ends with `orNull`. So I think the API should return `null` instead of raising an exception. We could use `ClientRequestContext.roo()` to find out if the client call is invoked by a server request or not. Modification: - Returns `null` instread of raising an exception from `(Service|Client)RequestContext.currentOrNull()` Result: - `(Service|Client)RequestContext.currentOrNull()` does not throw an exception anymore.
Codecov Report
@@ Coverage Diff @@
## master #2962 +/- ##
============================================
- Coverage 73.24% 73.18% -0.06%
- Complexity 12211 12213 +2
============================================
Files 1062 1058 -4
Lines 47343 47380 +37
Branches 5979 5995 +16
============================================
Hits 34675 34675
- Misses 9636 9670 +34
- Partials 3032 3035 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @minwoox
core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java
Outdated
Show resolved
Hide resolved
line#2962) * Fix not to raise an exception from `(Service|Client)RequestContext.currentOrNull()` Motivation: `ServiceRequestContext.currentOrNull()` raises an `IllegalArgumentException` if the current ctx is `ClientRequestContext` and `ctx.root()` returns `null`. It's very confusing because all other APIs returns `null` if the name ends with `orNull`. So I think the API should return `null` instead of raising an exception. We could use `ClientRequestContext.roo()` to find out if the client call is invoked by a server request or not. Modification: - Returns `null` instread of raising an exception from `(Service|Client)RequestContext.currentOrNull()` Result: - `(Service|Client)RequestContext.currentOrNull()` does not throw an exception anymore. * Address the comment by @trustin
…rrentOrNull()`
Motivation:
ServiceRequestContext.currentOrNull()
raises anIllegalArgumentException
if the current ctx is
ClientRequestContext
andctx.root()
returnsnull
.It's very confusing because all other APIs returns
null
if the name ends withorNull
.So I think the API should return
null
instead of raising an exception.We could use
ClientRequestContext.roo()
to find out if the client call is invoked by a server request or not.Modification:
null
instread of raising an exception from(Service|Client)RequestContext.currentOrNull()
Result:
(Service|Client)RequestContext.currentOrNull()
does not throw an exception anymore.