Skip to content

Commit

Permalink
Fix not to raise an exception from `(Service|Client)RequestContext.cu… (
Browse files Browse the repository at this point in the history
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
  • Loading branch information
minwoox committed Aug 3, 2020
1 parent fdc57a7 commit ba1930e
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 20 deletions.
Expand Up @@ -74,17 +74,14 @@ static ClientRequestContext current() {
* Returns the client-side context of the {@link Request} that is being handled in the current thread.
*
* @return the {@link ClientRequestContext} available in the current thread, or {@code null} if unavailable.
* @throws IllegalStateException if the current context is not a {@link ClientRequestContext}.
*/
@Nullable
static ClientRequestContext currentOrNull() {
final RequestContext ctx = RequestContext.currentOrNull();
if (ctx == null) {
return null;
if (ctx instanceof ClientRequestContext) {
return (ClientRequestContext) ctx;
}
checkState(ctx instanceof ClientRequestContext,
"The current context is not a client-side context: %s", ctx);
return (ClientRequestContext) ctx;
return null;
}

/**
Expand All @@ -105,6 +102,12 @@ static <T> T mapCurrent(
return mapper.apply(ctx);
}

final ServiceRequestContext serviceRequestContext = ServiceRequestContext.currentOrNull();
if (serviceRequestContext != null) {
throw new IllegalStateException("The current context is not a client-side context: " +
serviceRequestContext);
}

if (defaultValueSupplier != null) {
return defaultValueSupplier.get();
}
Expand Down
Expand Up @@ -91,14 +91,7 @@ static ServiceRequestContext currentOrNull() {
return null;
}

final ServiceRequestContext root = ctx.root();
if (root != null) {
return root;
}

throw new IllegalStateException(
"The current context is not a server-side context and does not have a root " +
"which means that the context is not invoked by a server request. ctx: " + ctx);
return ctx.root();
}

/**
Expand All @@ -119,6 +112,14 @@ static <T> T mapCurrent(
return mapper.apply(ctx);
}

final ClientRequestContext clientRequestContext = ClientRequestContext.currentOrNull();
if (clientRequestContext != null) {
throw new IllegalStateException(
"The current context is not a server-side context and does not have a root " +
"which means that the context is not invoked by a server request. ctx: " +
clientRequestContext);
}

if (defaultValueSupplier != null) {
return defaultValueSupplier.get();
}
Expand Down
Expand Up @@ -62,9 +62,7 @@ void currentOrNull() {
assertCurrentCtx(null);

try (SafeCloseable unused = serviceRequestContext().push()) {
assertThatThrownBy(ClientRequestContext::currentOrNull)
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("not a client-side context");
assertThat(ClientRequestContext.currentOrNull()).isNull();
}
}

Expand Down
Expand Up @@ -75,9 +75,7 @@ void currentOrNull() {
assertCurrentCtx(null);

try (SafeCloseable unused = clientRequestContext().push()) {
assertThatThrownBy(ServiceRequestContext::currentOrNull)
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("not a server-side context");
assertThat(ServiceRequestContext.currentOrNull()).isNull();
}
}

Expand Down

0 comments on commit ba1930e

Please sign in to comment.