-
Notifications
You must be signed in to change notification settings - Fork 38.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve WebClientResponseException message in case of 1xx/2xx/3xx status
When a response fails to be completely emitted by the remote (connection termination during the transmission of the response for example), a WebClientResponseException can be propagated with a confusing message which mainly reflects the status code and reason phrase, leading to messages like "200 OK" in such an exception. This change improves the situation by appending a hint at the underlying cause whenever getMessage() is called on a WebClientResponseException which was created with a non-error status code. Closes gh-33127
- Loading branch information
1 parent
dfa6b4b
commit 06d267f
Showing
2 changed files
with
100 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,19 @@ public void setBodyDecodeFunction(Function<ResolvableType, ?> decoderFunction) { | |
this.bodyDecodeFunction = decoderFunction; | ||
} | ||
|
||
@Override | ||
public String getMessage() { | ||
if (shouldHintAtResponseFailure()) { | ||
return super.getMessage() + ", but response failed with cause: " + getCause(); | ||
} | ||
return super.getMessage(); | ||
} | ||
|
||
private boolean shouldHintAtResponseFailure() { | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
simonbasle
Author
Contributor
|
||
return this.statusCode.is1xxInformational() || | ||
this.statusCode.is2xxSuccessful() || | ||
this.statusCode.is3xxRedirection(); | ||
} | ||
|
||
/** | ||
* Create {@code WebClientResponseException} or an HTTP status specific subclass. | ||
|
87 changes: 87 additions & 0 deletions
87
...ava/org/springframework/web/reactive/function/client/WebClientResponseExceptionTests.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/* | ||
* Copyright 2002-2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.web.reactive.function.client; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
class WebClientResponseExceptionTests { | ||
|
||
@Test | ||
void constructWithSuccessStatusCodeAndNoCauseAdditionalMessage() { | ||
assertThat(new WebClientResponseException(200, "OK", null, null, null)) | ||
.hasNoCause() | ||
.hasMessage("200 OK, but response failed with cause: null"); | ||
} | ||
|
||
@Test | ||
void constructWith1xxStatusCodeAndCauseAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException(100, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("100 reasonPhrase, but response failed with cause: java.lang.RuntimeException: example cause"); | ||
} | ||
|
||
@Test | ||
void constructWith2xxStatusCodeAndCauseAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException(200, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("200 reasonPhrase, but response failed with cause: java.lang.RuntimeException: example cause"); | ||
} | ||
|
||
@Test | ||
void constructWith3xxStatusCodeAndCauseAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException(300, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("300 reasonPhrase, but response failed with cause: java.lang.RuntimeException: example cause"); | ||
} | ||
|
||
@Test | ||
void constructWithExplicitMessageAndNotErrorCodeAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException("explicit message", 100, "reasonPhrase", null, null, null); | ||
assertThat(ex).hasMessage("explicit message, but response failed with cause: null"); | ||
} | ||
|
||
@Test | ||
void constructWithExplicitMessageAndNotErrorCodeAndCauseAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException("explicit message", 100, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("explicit message, but response failed with cause: java.lang.RuntimeException: example cause") | ||
.hasRootCauseMessage("example cause"); | ||
} | ||
|
||
@Test | ||
void constructWithExplicitMessageAndErrorCodeAndCauseNoAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException("explicit message", 404, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("explicit message").hasRootCauseMessage("example cause"); | ||
} | ||
|
||
@Test | ||
void constructWith4xxStatusCodeAndCauseNoAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException(400, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("400 reasonPhrase").hasRootCauseMessage("example cause"); | ||
} | ||
|
||
@Test | ||
void constructWith5xxStatusCodeAndCauseNoAdditionalMessage() { | ||
final WebClientResponseException ex = new WebClientResponseException(500, "reasonPhrase", null, null, null); | ||
ex.initCause(new RuntimeException("example cause")); | ||
assertThat(ex).hasMessage("500 reasonPhrase").hasRootCauseMessage("example cause"); | ||
} | ||
} |
This will cause issues in Mockito tests where just a
WebResponseExcepiton
is thrown for exampleWhen the Test mocks the following way:
The code uses the message of the web client exception in a catch block the following way:
a
NullPointerException
is thrown.I know you assume the status code is present because it should be in case of a
WebClientResponseException
but to make the code more test-friendly you could rewrite the if statement withBest Wishes,
Nico