feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException #1027

Luthan95 · 2023-03-20T06:26:39Z

add the getErrorBodyAs() method to SpinnakerServerException which returns the error response body as Map to handle the exception.

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

… response error payload as Map

…pinnakerServerException

…o support retrofit2 changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

…tead of multiple calls

dbyron-sf · 2023-05-08T19:51:20Z

...retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitException.java

@@ -69,8 +69,8 @@ public Response getResponse() {
 * @throws RuntimeException wrapping the underlying IOException if unable to convert the body to
 * the specified {@code type}.
 */
- public <T> T getErrorBodyAs(Class<T> type) {
- if (response == null || response.errorBody() == null) {


We still need the null check, yes? Do we have a test that covers this?

ya, SpinnakerRetrofit2ErrorHandleTest covers that scenario

Can you link to the specific test please? I don't see anything in SpinnakerRetrofit2ErrorHandleTest that calls getErrorBodyAs. It's hard to tell which test constructs a SpinnakerServerException with a RetrofitException built with a successful Response (i.e. one where response.errorBody is null here).

I suspect this is easier to test without all the rest endpoint stuff, like with a new RetrofitExceptionTest class.

Added test cases to RetrofitException and tried for Response.errorBody as null, but it is impossible to set response without error body, testcases helps to show those scenarios if we create response success response with otherthan success code it will throw IllegalArgumentException...

5f047fe shows that it is possible.

Perhaps a better solution is to make the constructor private? And then assert that Response.errorBody() is not null in the constructor?

Done in f0d5abe

… ...)

…t involve kork

dbyron-sf · 2023-05-19T19:47:24Z

...retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitException.java

@@ -69,10 +69,11 @@ public Response getResponse() {
 * @throws RuntimeException wrapping the underlying IOException if unable to convert the body to
 * the specified {@code type}.
 */
- public <T> T getErrorBodyAs(Class<T> type) {


This is a comment about an earlier line in this file, not modified (yet) in this PR. As I stare at this code some more, I noticed that Response is a generic type...So, instead of

private final Response response;

above, it's more correct to have

private final Response<T> response;

and then add T as a type parameter to RetrofitException and the other places Response is used.

We'd need a different letter here if we use T for that. Perhaps let's leave this for a followup PR.

Or maybe Response<?> is more appropriate?...

so construction only happens via the RetrofitException's factory methods

…on.getResponseBody

dbyron-sf · 2023-05-22T15:48:14Z

...c/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java

+ SpinnakerServerException serverException = new SpinnakerServerException(retrofitException);
+
+ Map<String, Object> responseBody = serverException.getResponseBody();
+ assertEquals(responseBody, null);


Suggested change

assertEquals(responseBody, null);

assertNull(responseBody);

dbyron-sf · 2023-05-22T15:48:27Z

...c/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java

+ SpinnakerServerException serverException = new SpinnakerServerException(retrofitException);
+
+ Map<String, Object> responseBody = serverException.getResponseBody();
+ assertEquals(responseBody, null);


Suggested change

assertEquals(responseBody, null);

assertNull(responseBody);

dbyron-sf · 2023-05-22T15:51:54Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ "{\"name\"=\"test\"}")),
+ retrofit2Service);
+
+ assertThrows(RuntimeException.class, () -> retrofitException.getBodyAs(HashMap.class));


I could use some comments here, or perhaps a more descriptive method name. Why does this test expect a RuntimeException?

dbyron-sf · 2023-05-22T15:53:54Z

...c/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java

+ .addConverterFactory(JacksonConverterFactory.create())
+ .build();
+
+ RetrofitException retrofitException =


I'd like to see some more test coverage here, and potentially in RetrofitExceptionTest as well -- of http responses with non-200 errors codes that name non-json response bodies, as well as a json list.

…on and RetrofitException

dbyron-sf · 2023-05-31T15:44:56Z

...t/java/com/netflix/spinnaker/kork/retrofit/exceptions/ByteArrayConverterFactoryTestUtil.java

+import retrofit2.Converter;
+import retrofit2.Retrofit;
+
+public class ByteArrayConverterFactoryTestUtil extends Converter.Factory {


javadoc please

dbyron-sf · 2023-05-31T15:46:32Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

@@ -78,6 +89,8 @@ public void testHttpConversionFail() {
 "{\"name\"=\"test\"}")),
 retrofit2Service);

+ // Gives MalformedJsonException or JsonEOFException or IllegalStateException, or
+ // some other exception for json parsing, which is a type of RuntimeException.


I still don't understand why to expect these exceptions. Isn't the response body valid json?

dbyron-sf · 2023-05-31T15:48:12Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ ResponseBody errorBody = response.errorBody();
+ assertNull(errorBody);
+ assertThrows(
+ NullPointerException.class, () -> RetrofitException.httpError(response, retrofit2Service));


Could use some comments here explaining why to expect a NullPointerException.

dbyron-sf · 2023-05-31T15:48:50Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ String body = response.body();
+ assertNotNull(body);
+
+ // Assert the response body


It's clear from the code that this is what's happening. What's not clear is why.

dbyron-sf · 2023-05-31T15:49:32Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ }
+
+ @Test
+ public void test2xxSuccessResponseWithWildcard() {


What additional coverage does this test give beyond test2xxSuccessResponseWithType?

dbyron-sf · 2023-05-31T15:50:06Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ this.testApiServiceForJsonResponse(expectedContentBodyMap, jsonContentString, null);
+ }
+
+ private void testApiServiceForJsonResponse(


javadoc please

dbyron-sf · 2023-05-31T15:50:47Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ assertNotNull(jsonContentString);
+
+ String invalidContentJsonBodyString = (String) jsonContentString;
+ // validate contents cannot be null, otherwise it gives NullPointerException


remove duplicate comment?

dbyron-sf · 2023-05-31T15:51:35Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ Response<?> actualResponse = Response.error(HttpStatus.UNAUTHORIZED.value(), responseBody);
+ assertNotNull(actualResponse);
+
+ // assert response body is null


I'm not sure we need to worry about actualResponse.body() since RetrofitException doesn't use it.

dbyron-sf · 2023-05-31T15:52:24Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ // Use default as GsonConverterFactory if not specified, for json list with lenient option to
+ // allow invalid json strings also
+ if (converterFactoryList == null || converterFactoryList.size() == 0) {
+ com.google.gson.Gson gson = new GsonBuilder().setLenient().create();


Create this once and store in a variable outside this function?

dbyron-sf · 2023-05-31T15:53:15Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ Retrofit.Builder customRetrofitBuilder =
+ new Retrofit.Builder().baseUrl("http://localhost:8987/");
+
+ // Use default as GsonConverterFactory if not specified, for json list with lenient option to


Suggested change

// Use default as GsonConverterFactory if not specified, for json list with lenient option to

// Use GsonConverterFactory if not specified, for json list with lenient option to

Please also explain the choice of GsonConverterFactory (it may be arbitrary). Is it the only one with a lenient option?

dbyron-sf · 2023-05-31T15:54:09Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ for (Object key : expectedContentBodyMap.keySet()) {
+ assertEquals(expectedContentBodyMap.get(key), actualResponseBodyContentMap.get(key));
+ }
+ } catch (RuntimeException exception) {


Not sure what this catch-and-rethrow adds.

dbyron-sf · 2023-05-31T15:55:56Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ assertEquals(type, "UNKOWN_IOEXCEPTION_WHILE_SERVER_COMMUNICATION");
+
+ Map<String, String> responseBodyMap = retrofitException.getBodyAs(HashMap.class);
+ assertNull(responseBodyMap);


Why expect null here?

dbyron-sf · 2023-05-31T15:56:32Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ MediaType.parse("application/json" + "; charset=utf-8"), "{\"name\":\"test\"}");
+ Response<?> actualResponse = Response.error(HttpStatus.INTERNAL_SERVER_ERROR.value(), body);
+ assertNotNull(actualResponse);
+ networkException.initCause(new retrofit2.HttpException(actualResponse));


Why is it important to do this?

dbyron-sf · 2023-05-31T15:58:27Z

...ofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/RetrofitExceptionTest.java

+ }
+
+ @Test
+ public void testOrderOfConverterFactory() throws JsonProcessingException {


Not sure we need to test retrofit2 internals. What would we do if this test starts failing?

dbyron-sf · 2023-05-31T16:00:04Z

...t/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java

+ String cause = exception.getCause().getMessage();
+ assertNotNull(cause);
+ assertNull(exception.getRetryable());
+ assertTrue(cause.contains("timeout"));


What is this assertion telling us?

dbyron-sf · 2023-05-31T16:01:28Z

...t/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java

+ // create a simple invalid json string
+ // "{\"name\":\"te";// incomplete json gives com.fasterxml.jackson.core.io.JsonEOFException
+ String responseBodyString =
+ "{\"name\"=\"test\"}"; // com.fasterxml.jackson.core.JsonParseException


Doesn't match the comments.

dbyron-sf · 2023-05-31T16:36:54Z

...t/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java

+ }
+
+ @Test
+ public void test200ResponseWithInvalidJson() throws Exception {


Please also add a test of non-2xx (e.g. 500) response with invalid json.

…on and RetrofitException

Luthan95 force-pushed the customexception_retrofitErrorbody branch from c171653 to cbfa0ed Compare March 20, 2023 06:27

Luthan95 changed the title ~~refactor(SpinnakerServerException): add method which returns retrofit…~~ refactor(SpinnakerServerException): create method which returns retrofit error response body as Map Mar 20, 2023

Luthan95 changed the title ~~refactor(SpinnakerServerException): create method which returns retrofit error response body as Map~~ refactor(retrofit): add getErrorBodyAs() method to SpinnakerServerException Mar 20, 2023

dbyron-sf reviewed Mar 22, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Mar 22, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Mar 22, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Mar 24, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

Luthan95 force-pushed the customexception_retrofitErrorbody branch from e497f0b to f610a6b Compare April 28, 2023 07:27

Luthan95 added 3 commits May 3, 2023 12:42

refactor(SpinnakerServerException): add method which returns retrofit…

ed04661

… response error payload as Map

refactor(SpinnakerServerException): add getResponseBody() method to S…

d48a76b

…pinnakerServerException

refactor(SpinnakerServerException): modify SpinnakerServerException t…

da6cc53

…o support retrofit2 changes

Luthan95 force-pushed the customexception_retrofitErrorbody branch from f610a6b to da6cc53 Compare May 3, 2023 07:50

dbyron-sf reviewed May 8, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Show resolved Hide resolved

Luthan95 force-pushed the customexception_retrofitErrorbody branch from 91cb58a to 9b8d7e0 Compare May 8, 2023 10:47

dbyron-sf reviewed May 8, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

dbyron-sf reviewed May 8, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

dbyron-sf reviewed May 8, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Outdated Show resolved Hide resolved

refactor(SpinnakerServerException): set message from responseBody ins…

554f290

…tead of multiple calls

Luthan95 force-pushed the customexception_retrofitErrorbody branch from 9b8d7e0 to 554f290 Compare May 8, 2023 13:36

dbyron-sf reviewed May 8, 2023

View reviewed changes

Luthan95 and others added 4 commits May 19, 2023 20:01

refactor(RetrofitException): add test cases to RetrofitException

91b473f

fix(retrofit): handle a null errorBody in RetrofitException.getBodyAs

5f047fe

refactor(retrofit/test): use assertNull instead of assertEquals(null,…

8b01978

… ...)

refactor(retrofit/test): remove tests of retrofit2 behavior that don'…

e4a76d5

…t involve kork

dbyron-sf reviewed May 19, 2023

View reviewed changes

dbyron-sf and others added 2 commits May 19, 2023 14:11

refactor(retrofit): make the RetrofitException constructor private

f0d5abe

so construction only happens via the RetrofitException's factory methods

refactor(retrofit/test): add test coverage for SpinnakerServerExcepti…

0018064

…on.getResponseBody

Luthan95 force-pushed the customexception_retrofitErrorbody branch from 7fc44db to 0018064 Compare May 22, 2023 06:24

dbyron-sf reviewed May 22, 2023

View reviewed changes

refactor(retrofit/test): add test coverage for SpinnakerServerExcepti…

09e60f8

…on and RetrofitException

dbyron-sf reviewed May 31, 2023

View reviewed changes

refactor(retrofit/test): add test coverage for SpinnakerServerExcepti…

bac71c4

…on and RetrofitException

dbyron-sf changed the title ~~refactor(retrofit): add getErrorBodyAs() method to SpinnakerServerException~~ feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException Jun 5, 2023

dbyron-sf closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException #1027

feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException #1027

Luthan95 commented Mar 20, 2023 •

edited

Loading

dbyron-sf May 8, 2023

Luthan95 May 9, 2023

dbyron-sf May 11, 2023

dbyron-sf May 11, 2023

Luthan95 May 19, 2023 •

edited

Loading

dbyron-sf May 19, 2023

dbyron-sf May 19, 2023

dbyron-sf May 19, 2023

dbyron-sf May 19, 2023

dbyron-sf May 19, 2023

dbyron-sf May 22, 2023

dbyron-sf May 22, 2023

dbyron-sf May 22, 2023

dbyron-sf May 22, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023

dbyron-sf May 31, 2023 •

edited

Loading

	// Use default as GsonConverterFactory if not specified, for json list with lenient option to
	// Use GsonConverterFactory if not specified, for json list with lenient option to

feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException #1027

feat(retrofit): add getErrorBodyAs() method to SpinnakerServerException #1027

Conversation

Luthan95 commented Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthan95 May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbyron-sf May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Luthan95 commented Mar 20, 2023 •

edited

Loading

Luthan95 May 19, 2023 •

edited

Loading

dbyron-sf May 31, 2023 •

edited

Loading