From b4d1b599fbab7a7712330c6703120084db2bee5e Mon Sep 17 00:00:00 2001 From: SheetalAtre Date: Mon, 7 Aug 2023 23:01:00 +0530 Subject: [PATCH] refactor(retrofit/test): rebase with master branch --- .../exceptions/SpinnakerHttpException.java | 42 +++++++---------- .../SpinnakerHttpExceptionTest.java | 45 ++++++++++--------- .../SpinnakerServerExceptionTest.java | 18 -------- 3 files changed, 42 insertions(+), 63 deletions(-) diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java index 75137277c..7e9a46c2d 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java @@ -50,9 +50,6 @@ public class SpinnakerHttpException extends SpinnakerServerException { private final Map responseBody; private final retrofit2.Retrofit retrofit; - private final String retrofit2DefaultResponseError = "Response.error()"; - private final String errorMessageFormat = "Status: %s, URL: %s, Message: %s"; - private final String messageKey = "message"; public SpinnakerHttpException(RetrofitError e) { super(e); @@ -108,12 +105,15 @@ public SpinnakerHttpException( this.retrofit2Response = retrofit2Response; this.response = null; this.retrofit = retrofit; - this.rawMessage = null; if ((retrofit2Response.code() == HttpStatus.NOT_FOUND.value()) || (retrofit2Response.code() == HttpStatus.BAD_REQUEST.value())) { setRetryable(false); } responseBody = (Map) this.getErrorBodyAs(HashMap.class); + this.rawMessage = + responseBody != null + ? (String) responseBody.getOrDefault("message", getMessage()) + : getMessage(); } public int getResponseCode() { @@ -144,32 +144,24 @@ public HttpHeaders getHeaders() { @Override public String getMessage() { - if (retrofit2Response != null) { - String defaultMessage = retrofit2Response.code() + " " + retrofit2Response.message(); - String msg = - responseBody != null - ? (String) responseBody.getOrDefault(messageKey, defaultMessage) - : defaultMessage; - - if (!retrofit2DefaultResponseError.equals(retrofit2Response.message())) { - msg = defaultMessage; // means message property is not set in the error response body - } + // If there's no message derived from a response, get the specified message. + // It feels a little backwards to do it this way, but super.getMessage() + // always returns something whether there's a specified message or not, so + // look at getRawMessage instead. + if (getRawMessage() == null) { + return super.getMessage(); + } + if (retrofit2Response != null) { return String.format( - errorMessageFormat, + "Status: %s, URL: %s, Message: %s", retrofit2Response.code(), - retrofit2Response.raw().request().url(), - msg); + retrofit2Response.raw().request().url().toString(), + getRawMessage()); } else { - // If there's no message derived from a response, get the specified message. - // It feels a little backwards to do it this way, but super.getMessage() - // always returns something whether there's a specified message or not, so - // look at getRawMessage instead. - if (getRawMessage() == null) { - return super.getMessage(); - } return String.format( - errorMessageFormat, response.getStatus(), response.getUrl(), getRawMessage()); + "Status: %s, URL: %s, Message: %s", + response.getStatus(), response.getUrl(), getRawMessage()); } } diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java index 0f965b740..d8e8e1f07 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java @@ -12,25 +12,32 @@ * 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 com.netflix.spinnaker.kork.retrofit.exceptions; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import com.netflix.spinnaker.kork.exceptions.SpinnakerException; +import java.util.List; import java.util.Map; import okhttp3.MediaType; import okhttp3.ResponseBody; import org.junit.jupiter.api.Test; import org.springframework.http.HttpStatus; +import retrofit.RetrofitError; +import retrofit.client.Response; import retrofit2.Retrofit; import retrofit2.converter.jackson.JacksonConverterFactory; public class SpinnakerHttpExceptionTest { + private static final String CUSTOM_MESSAGE = "custom message"; @Test - public void testSpinnakerHttpException() { + public void testSpinnakerHttpExceptionFromRetrofitException() { final String validJsonResponseBodyString = "{\"name\":\"test\"}"; ResponseBody responseBody = ResponseBody.create( @@ -49,25 +56,23 @@ public void testSpinnakerHttpException() { Map errorResponseBody = notFoundException.getResponseBody(); assertEquals(errorResponseBody.get("name"), "test"); assertEquals(HttpStatus.NOT_FOUND.value(), notFoundException.getResponseCode()); + assertTrue( + notFoundException.getMessage().contains(String.valueOf(HttpStatus.NOT_FOUND.value()))); + } + + @Test + public void testSpinnakerHttpException_NewInstance() { + Response response = new Response("http://localhost", 200, "reason", List.of(), null); + try { + RetrofitError error = RetrofitError.httpError("http://localhost", response, null, null); + throw new SpinnakerHttpException(error); + } catch (SpinnakerException e) { + SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - // Note: - // * If error response body has property "message" then return its value: - // eg: if errorResponseBody = {"message":"test"}, then expectedMessage = "Status: 404, URL: - // http://localhost/, Message: test" - // * If error response body does not have any property "message", then return default error - // message: - // eg: if errorResponseBody = {"name":"test"}, then expectedMessage = "Status: 404, URL: - // http://localhost/, Message: 404 Response.error()" - // * TODO: If error response body is an invalid json then the converter fails to convert it and - // returns error message: - // eg: if errorResponseBody = {"name":"test",(no closing bracket at end), then expectedMessage - // ="Failed to parse response". - String expectedMessage = - String.format( - "Status: %s, URL: %s, Message: %s", - HttpStatus.NOT_FOUND.value(), - "http://localhost/", - HttpStatus.NOT_FOUND.value() + " " + "Response.error()"); - assertEquals(expectedMessage, notFoundException.getMessage()); + assertTrue(newException instanceof SpinnakerHttpException); + assertEquals(CUSTOM_MESSAGE, newException.getMessage()); + assertEquals(e, newException.getCause()); + assertEquals(response.getStatus(), ((SpinnakerHttpException) newException).getResponseCode()); + } } } diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java index 9b74866a4..a9b764a93 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java @@ -21,30 +21,12 @@ import com.netflix.spinnaker.kork.exceptions.SpinnakerException; import java.io.IOException; -import java.util.List; import org.junit.jupiter.api.Test; import retrofit.RetrofitError; -import retrofit.client.Response; public class SpinnakerServerExceptionTest { private static final String CUSTOM_MESSAGE = "custom message"; - @Test - public void testSpinnakerHttpException_NewInstance() { - Response response = new Response("http://localhost", 200, "reason", List.of(), null); - try { - RetrofitError error = RetrofitError.httpError("http://localhost", response, null, null); - throw new SpinnakerHttpException(error); - } catch (SpinnakerException e) { - SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - - assertTrue(newException instanceof SpinnakerHttpException); - assertEquals(CUSTOM_MESSAGE, newException.getMessage()); - assertEquals(e, newException.getCause()); - assertEquals(response.getStatus(), ((SpinnakerHttpException) newException).getResponseCode()); - } - } - @Test public void testSpinnakerNetworkException_NewInstance() { IOException initialException = new IOException("message");