From a69e3be3e1bb9132e639ac001488807ba1483345 Mon Sep 17 00:00:00 2001 From: Oleksandr Zhevedenko <720803+Net-burst@users.noreply.github.com> Date: Fri, 13 Sep 2024 09:33:40 -0400 Subject: [PATCH] Bugfix: RemoteFileSyncer handling of error responses (#3440) --- .../server/execution/RemoteFileSyncer.java | 31 ++++- .../execution/RemoteFileSyncerTest.java | 108 ++++++++++++++++-- 2 files changed, 123 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/prebid/server/execution/RemoteFileSyncer.java b/src/main/java/org/prebid/server/execution/RemoteFileSyncer.java index 9a6416ba44c..b841bf8a136 100644 --- a/src/main/java/org/prebid/server/execution/RemoteFileSyncer.java +++ b/src/main/java/org/prebid/server/execution/RemoteFileSyncer.java @@ -1,5 +1,6 @@ package org.prebid.server.execution; +import io.netty.handler.codec.http.HttpResponseStatus; import io.vertx.core.Future; import io.vertx.core.Promise; import io.vertx.core.Vertx; @@ -69,12 +70,14 @@ public RemoteFileSyncer(RemoteFileProcessor processor, getFileRequestOptions = new RequestOptions() .setMethod(HttpMethod.GET) .setTimeout(timeout) - .setAbsoluteURI(downloadUrl); + .setAbsoluteURI(downloadUrl) + .setFollowRedirects(true); isUpdateRequiredRequestOptions = new RequestOptions() .setMethod(HttpMethod.HEAD) .setTimeout(timeout) - .setAbsoluteURI(downloadUrl); + .setAbsoluteURI(downloadUrl) + .setFollowRedirects(true); } private static void createAndCheckWritePermissionsFor(FileSystem fileSystem, String filePath) { @@ -112,8 +115,7 @@ private Future deleteFile(String filePath) { private Future syncRemoteFile(RetryPolicy retryPolicy) { return fileSystem.open(tmpFilePath, new OpenOptions()) - .compose(tmpFile -> httpClient.request(getFileRequestOptions) - .compose(HttpClientRequest::send) + .compose(tmpFile -> sendHttpRequest(getFileRequestOptions) .compose(response -> response.pipeTo(tmpFile)) .onComplete(result -> tmpFile.close())) @@ -148,8 +150,7 @@ private void setUpDeferredUpdate() { } private void updateIfNeeded() { - httpClient.request(isUpdateRequiredRequestOptions) - .compose(HttpClientRequest::send) + sendHttpRequest(isUpdateRequiredRequestOptions) .compose(response -> fileSystem.exists(saveFilePath) .compose(exists -> exists ? isLengthChanged(response) @@ -161,6 +162,24 @@ private void updateIfNeeded() { }); } + private Future sendHttpRequest(RequestOptions requestOptions) { + return httpClient.request(requestOptions) + .compose(HttpClientRequest::send) + .compose(this::validateResponse); + } + + private Future validateResponse(HttpClientResponse response) { + final int statusCode = response.statusCode(); + if (statusCode != HttpResponseStatus.OK.code()) { + return Future.failedFuture(new PreBidException( + String.format("Got unexpected response from server with status code %s and message %s", + statusCode, + response.statusMessage()))); + } else { + return Future.succeededFuture(response); + } + } + private Future isLengthChanged(HttpClientResponse response) { final String contentLengthParameter = response.getHeader(HttpHeaders.CONTENT_LENGTH); return StringUtils.isNumeric(contentLengthParameter) && !contentLengthParameter.equals("0") diff --git a/src/test/java/org/prebid/server/execution/RemoteFileSyncerTest.java b/src/test/java/org/prebid/server/execution/RemoteFileSyncerTest.java index 341c7764cb3..9acd719d30f 100644 --- a/src/test/java/org/prebid/server/execution/RemoteFileSyncerTest.java +++ b/src/test/java/org/prebid/server/execution/RemoteFileSyncerTest.java @@ -1,5 +1,6 @@ package org.prebid.server.execution; +import io.netty.handler.codec.http.HttpResponseStatus; import io.vertx.core.Future; import io.vertx.core.Handler; import io.vertx.core.Vertx; @@ -182,6 +183,8 @@ public void syncForFilepathShouldNotUpdateWhenHeadRequestReturnInvalidHead() { .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) .willReturn(Future.succeededFuture(httpClientResponse)); + given(httpClientResponse.statusCode()) + .willReturn(HttpResponseStatus.OK.code()); given(httpClientResponse.getHeader(HttpHeaders.CONTENT_LENGTH)) .willReturn("notnumber"); @@ -209,7 +212,10 @@ public void syncForFilepathShouldNotUpdateWhenPropsIsFailed() { .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) .willReturn(Future.succeededFuture(httpClientResponse)); - given(httpClientResponse.getHeader(any(CharSequence.class))).willReturn(FILE_SIZE.toString()); + given(httpClientResponse.statusCode()) + .willReturn(HttpResponseStatus.OK.code()); + given(httpClientResponse.getHeader(any(CharSequence.class))) + .willReturn(FILE_SIZE.toString()); given(fileSystem.props(anyString())) .willReturn(Future.failedFuture(new IllegalArgumentException("ERROR"))); @@ -240,7 +246,10 @@ public void syncForFilepathShouldNotUpdateServiceWhenSizeEqualsContentLength() { .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) .willReturn(Future.succeededFuture(httpClientResponse)); - given(httpClientResponse.getHeader(any(CharSequence.class))).willReturn(FILE_SIZE.toString()); + given(httpClientResponse.statusCode()) + .willReturn(HttpResponseStatus.OK.code()); + given(httpClientResponse.getHeader(any(CharSequence.class))) + .willReturn(FILE_SIZE.toString()); given(fileSystem.props(anyString())) .willReturn(Future.succeededFuture(fileProps)); @@ -274,8 +283,12 @@ public void syncForFilepathShouldUpdateServiceWhenSizeNotEqualsContentLength() { .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) .willReturn(Future.succeededFuture(httpClientResponse)); - given(httpClientResponse.pipeTo(any())).willReturn(Future.succeededFuture()); - given(httpClientResponse.getHeader(any(CharSequence.class))).willReturn(FILE_SIZE.toString()); + given(httpClientResponse.pipeTo(any())) + .willReturn(Future.succeededFuture()); + given(httpClientResponse.statusCode()) + .willReturn(HttpResponseStatus.OK.code()); + given(httpClientResponse.getHeader(any(CharSequence.class))) + .willReturn(FILE_SIZE.toString()); given(fileSystem.props(anyString())) .willReturn(Future.succeededFuture(fileProps)); @@ -291,7 +304,8 @@ public void syncForFilepathShouldUpdateServiceWhenSizeNotEqualsContentLength() { given(fileSystem.move(anyString(), any(), any(CopyOptions.class))) .willReturn(Future.succeededFuture()); - given(remoteFileProcessor.setDataPath(anyString())).willReturn(Future.succeededFuture()); + given(remoteFileProcessor.setDataPath(anyString())) + .willReturn(Future.succeededFuture()); // when remoteFileSyncer.sync(); @@ -354,7 +368,8 @@ public void syncForFilepathShouldRetryWhenFileOpeningFailed() { .willAnswer(withSelfAndPassObjectToHandler(Future.succeededFuture())) .willAnswer(withSelfAndPassObjectToHandler(Future.failedFuture(new RuntimeException()))); - given(remoteFileProcessor.setDataPath(anyString())).willReturn(Future.succeededFuture()); + given(remoteFileProcessor.setDataPath(anyString())) + .willReturn(Future.succeededFuture()); // when remoteFileSyncer.sync(); @@ -370,7 +385,8 @@ public void syncForFilepathShouldRetryWhenFileOpeningFailed() { @Test public void syncForFilepathShouldDownloadFilesAndNotUpdateWhenUpdatePeriodIsNotSet() { // given - given(remoteFileProcessor.setDataPath(anyString())).willReturn(Future.succeededFuture()); + given(remoteFileProcessor.setDataPath(anyString())) + .willReturn(Future.succeededFuture()); given(fileSystem.exists(anyString())) .willReturn(Future.succeededFuture(false)); @@ -382,6 +398,8 @@ public void syncForFilepathShouldDownloadFilesAndNotUpdateWhenUpdatePeriodIsNotS .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) .willReturn(Future.succeededFuture(httpClientResponse)); + given(httpClientResponse.statusCode()) + .willReturn(HttpResponseStatus.OK.code()); given(httpClientResponse.pipeTo(asyncFile)) .willReturn(Future.succeededFuture()); @@ -395,6 +413,7 @@ public void syncForFilepathShouldDownloadFilesAndNotUpdateWhenUpdatePeriodIsNotS verify(fileSystem).open(eq(TMP_FILE_PATH), any()); verify(httpClient).request(any()); verify(asyncFile).close(); + verify(httpClientResponse).statusCode(); verify(remoteFileProcessor).setDataPath(any()); verify(fileSystem).move(eq(TMP_FILE_PATH), eq(FILE_PATH), any(CopyOptions.class)); verify(vertx, never()).setTimer(eq(UPDATE_INTERVAL), any()); @@ -419,8 +438,6 @@ public void syncForFilepathShouldRetryWhenTimeoutIsReached() { given(httpClient.request(any())) .willReturn(Future.succeededFuture(httpClientRequest)); given(httpClientRequest.send()) - .willReturn(Future.succeededFuture(httpClientResponse)); - given(httpClientResponse.pipeTo(asyncFile)) .willReturn(Future.failedFuture("Timeout")); // when @@ -429,6 +446,7 @@ public void syncForFilepathShouldRetryWhenTimeoutIsReached() { // then verify(vertx, times(RETRY_COUNT)).setTimer(eq(RETRY_INTERVAL), any()); verify(fileSystem, times(RETRY_COUNT + 1)).open(eq(TMP_FILE_PATH), any()); + verify(httpClientResponse, never()).pipeTo(any()); // Response handled verify(httpClient, times(RETRY_COUNT + 1)).request(any()); @@ -437,11 +455,81 @@ public void syncForFilepathShouldRetryWhenTimeoutIsReached() { verifyNoInteractions(remoteFileProcessor); } + @Test + public void syncShouldNotSaveFileWhenServerRespondsWithNonOkStatusCode() { + // given + given(fileSystem.exists(anyString())) + .willReturn(Future.succeededFuture(false)); + given(fileSystem.open(any(), any())) + .willReturn(Future.succeededFuture(asyncFile)); + given(fileSystem.move(anyString(), anyString(), any(CopyOptions.class))) + .willReturn(Future.succeededFuture()); + + given(httpClient.request(any())) + .willReturn(Future.succeededFuture(httpClientRequest)); + given(httpClientRequest.send()) + .willReturn(Future.succeededFuture(httpClientResponse)); + given(httpClientResponse.statusCode()) + .willReturn(0); + + // when + remoteFileSyncer.sync(); + + // then + verify(fileSystem, times(1)).exists(eq(FILE_PATH)); + verify(fileSystem).open(eq(TMP_FILE_PATH), any()); + verify(fileSystem).delete(eq(TMP_FILE_PATH)); + verify(asyncFile).close(); + verify(fileSystem, never()).move(eq(TMP_FILE_PATH), eq(FILE_PATH), any(CopyOptions.class)); + verify(httpClient).request(any()); + verify(httpClientResponse).statusCode(); + verify(httpClientResponse, never()).pipeTo(any()); + verify(remoteFileProcessor, never()).setDataPath(any()); + verify(vertx, never()).setTimer(eq(UPDATE_INTERVAL), any()); + } + + @Test + public void syncShouldNotUpdateFileWhenServerRespondsWithNonOkStatusCode() { + // given + remoteFileSyncer = new RemoteFileSyncer( + remoteFileProcessor, SOURCE_URL, FILE_PATH, TMP_FILE_PATH, RETRY_POLICY, + TIMEOUT, UPDATE_INTERVAL, httpClient, vertx); + + givenTriggerUpdate(); + + given(fileSystem.open(any(), any())) + .willReturn(Future.succeededFuture(asyncFile)); + given(fileSystem.move(anyString(), anyString(), any(CopyOptions.class))) + .willReturn(Future.succeededFuture()); + + given(httpClient.request(any())) + .willReturn(Future.succeededFuture(httpClientRequest)); + given(httpClientRequest.send()) + .willReturn(Future.succeededFuture(httpClientResponse)); + given(httpClientResponse.statusCode()) + .willReturn(0); + + // when + remoteFileSyncer.sync(); + + // then + verify(fileSystem, times(1)).exists(eq(FILE_PATH)); + verify(fileSystem, never()).open(any(), any()); + verify(fileSystem, never()).delete(any()); + verify(fileSystem, never()).move(any(), any(), any(), any()); + verify(asyncFile, never()).close(); + verify(httpClient, times(1)).request(any()); + verify(httpClientResponse).statusCode(); + verify(httpClientResponse, never()).pipeTo(any()); + verify(vertx).setPeriodic(eq(UPDATE_INTERVAL), any()); + } + private void givenTriggerUpdate() { given(fileSystem.exists(anyString())) .willReturn(Future.succeededFuture(true)); - given(remoteFileProcessor.setDataPath(anyString())).willReturn(Future.succeededFuture()); + given(remoteFileProcessor.setDataPath(anyString())) + .willReturn(Future.succeededFuture()); given(vertx.setPeriodic(eq(UPDATE_INTERVAL), any())) .willAnswer(withReturnObjectAndPassObjectToHandler(123L, 123L, 1))