Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import com.salesforce.einsteinbot.sdk.model.Error;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.ReactiveHttpInputMessage;
import org.springframework.http.client.reactive.ClientHttpResponse;
import org.springframework.web.reactive.function.BodyExtractor;
Expand All @@ -32,7 +34,6 @@
public class WebClientUtil {

private static final Logger logger = LoggerFactory.getLogger(WebClientUtil.class);

public static Mono<ClientRequest> createLoggingRequestProcessor(ClientRequest clientRequest) {
logger.info("Making {} Request to URI {} with Headers : {}", clientRequest.method(),
clientRequest.url(), maskAuthorizationHeader(clientRequest.headers()));
Expand Down Expand Up @@ -60,8 +61,9 @@ public static ExchangeFilterFunction createFilter(

public static BodyExtractor<Mono<Error>, ReactiveHttpInputMessage> errorBodyExtractor() {
BodyExtractor<Mono<Error>, ReactiveHttpInputMessage> extractor = (inputMessage, context) -> {
String contentType = inputMessage.getHeaders().getContentType().toString();
if (contentType.contains("application/json")) {
HttpHeaders headers = inputMessage.getHeaders();
MediaType contentType = headers != null ? headers.getContentType() : null;
if (contentType != null && contentType.toString().toLowerCase().contains("application/json")) {
return BodyExtractors.toMono(Error.class)
.extract(inputMessage, context);
} else {
Expand All @@ -72,10 +74,10 @@ public static BodyExtractor<Mono<Error>, ReactiveHttpInputMessage> errorBodyExtr
}

private static Mono<Error> buildErrorFromClientResponseBodyString(ReactiveHttpInputMessage clientResponse, BodyExtractor.Context context) {
ClientHttpResponse response = (ClientHttpResponse) clientResponse;
Mono<String> bodyString = BodyExtractors.toMono(String.class).
ClientHttpResponse response = (ClientHttpResponse) clientResponse;
Mono<String> bodyString = BodyExtractors.toMono(String.class).
extract(clientResponse, context);
return bodyString.map(errorMessage -> new Error()
return bodyString.map(errorMessage -> new Error()
.status(response.getRawStatusCode())
.message("This Response content type is not 'application/json', " +
"See the 'error' field for actual error returned by the server.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,41 @@ private void stubVersionsResponse(String responseBodyFile, int statusCode) {
);
}

}
@Test
public void test429ResponseWithNullContentType() {
wireMock.stubFor(
get(VERSIONS_URI)
.willReturn(
aResponse()
.withStatus(HttpStatus.TOO_MANY_REQUESTS.value())
.withBody("Too many requests")
)
);

Throwable exceptionThrown = assertThrows(RuntimeException.class,
() -> client.getSupportedVersions());

ChatbotResponseException chatbotResponseException = validateAndGetCause(exceptionThrown,
ChatbotResponseException.class);
assertEquals(HttpStatus.TOO_MANY_REQUESTS.value(), chatbotResponseException.getStatus());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... Why is it a 429?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are stubbing this response in line 416 along with a null contentType. This basically mocks the current flow where the sdk breaks due to NPE. Now the test verifies that the NPE issue is fixed for such responses

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I'm not convinced on the 429 part. If the issue is with content type, it should have been a 415.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is with a "null" content-type. GRLS metrics confirmed that it was throwing 429 and we faced this NPE during that time interval

}

@Test
public void test200ResponseWithApplicationJsonContentType() throws Exception {
String responseBodyFile = "versionsResponse.json";
wireMock.stubFor(
get(VERSIONS_URI)
.willReturn(
aResponse()
.withStatus(HttpStatus.OK.value())
.withHeader("Content-Type", "application/json;charset=UTF-8")
.withBodyFile(TEST_MOCK_DIR + responseBodyFile)
)
);

SupportedVersions versions = client.getSupportedVersions();

verifyResponseEnvelope(responseBodyFile, versions);
}

}
Loading