From 77ccd19c5b6941b1a5965ed6c65ea38c671b1c30 Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Wed, 8 Jan 2025 18:34:32 +0000 Subject: [PATCH 1/8] fix: Add logging to AbstractMethod.execute --- .../com/vonage/client/AbstractMethod.java | 50 +++++++++++++++---- .../vonage/client/auth/RequestSigning.java | 32 ++++++------ .../client/incoming/InputEventTest.java | 12 ++--- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/vonage/client/AbstractMethod.java b/src/main/java/com/vonage/client/AbstractMethod.java index a26a1a269..f7c9164cc 100644 --- a/src/main/java/com/vonage/client/AbstractMethod.java +++ b/src/main/java/com/vonage/client/AbstractMethod.java @@ -16,7 +16,6 @@ package com.vonage.client; import com.vonage.client.auth.*; -import org.apache.commons.logging.LogFactory; import org.apache.http.HttpHeaders; import org.apache.http.HttpResponse; import org.apache.http.client.methods.CloseableHttpResponse; @@ -26,26 +25,31 @@ import java.nio.charset.StandardCharsets; import java.util.AbstractMap; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; /** * Abstract class to assist in implementing a call against a REST endpoint. *

* Concrete implementations must implement {@link #makeRequest(Object)} to construct a {@link RequestBuilder} from the - * provided parameterized request object, and {@link #parseResponse(HttpResponse)} to construct the parameterized {@link - * HttpResponse} object. + * provided parameterized request object, and {@link #parseResponse(HttpResponse)} to construct the parameterized + * {@link HttpResponse} object. *

* The REST call is executed by calling {@link #execute(Object)}. * - * @param The type of the method-specific request object that will be used to construct an HTTP request - * @param The type of method-specific response object which will be constructed from the returned HTTP - * response + * @param The request object type that will be used to construct the HTTP request body. + * @param The response object type which will be constructed from the returned HTTP response body. + * + * @see DynamicEndpoint for an abstract implementation which handles the most common use cases. */ public abstract class AbstractMethod implements RestEndpoint { - static { - LogFactory.getLog(AbstractMethod.class); + private static final Logger LOGGER = Logger.getLogger(AbstractMethod.class.getName()); + + private static boolean shouldLog() { + return LOGGER.isLoggable(Level.INFO); } - + protected final HttpWrapper httpWrapper; public AbstractMethod(HttpWrapper httpWrapper) { @@ -75,15 +79,41 @@ public ResultT execute(RequestT request) throws VonageResponseParseException, Vo .setHeader(HttpHeaders.USER_AGENT, httpWrapper.getUserAgent()) .setCharset(StandardCharsets.UTF_8).build(); + if (shouldLog()) { + LOGGER.info("Request " + httpRequest.getMethod() + " " + httpRequest.getURI()); + StringBuilder headers = new StringBuilder("Request headers:\n"); + for (org.apache.http.Header header : httpRequest.getAllHeaders()) { + headers.append('\n').append(header.getName()).append(": ").append(header.getValue()); + } + LOGGER.info(headers.toString()); + LOGGER.info("Request body: " + request); + } + try (CloseableHttpResponse response = httpWrapper.getHttpClient().execute(httpRequest)) { try { - return postProcessParsedResponse(parseResponse(response)); + if (shouldLog()) { + LOGGER.info("Response " + response.getStatusLine()); + StringBuilder headers = new StringBuilder("Response headers:\n"); + for (org.apache.http.Header header : response.getAllHeaders()) { + headers.append('\n').append(header.getName()).append(": ").append(header.getValue()); + } + LOGGER.info(headers.toString()); + } + + ResultT responseBody = parseResponse(response); + if (shouldLog()) { + LOGGER.info("Response body: " + responseBody); + } + + return postProcessParsedResponse(responseBody); } catch (IOException iox) { + LOGGER.log(Level.WARNING, "Failed to parse response", iox); throw new VonageResponseParseException(iox); } } catch (IOException iox) { + LOGGER.log(Level.WARNING, "Failed to execute HTTP request", iox); throw new VonageMethodFailedException("Something went wrong while executing the HTTP request.", iox); } } diff --git a/src/main/java/com/vonage/client/auth/RequestSigning.java b/src/main/java/com/vonage/client/auth/RequestSigning.java index c09c0c6e7..a207583b4 100644 --- a/src/main/java/com/vonage/client/auth/RequestSigning.java +++ b/src/main/java/com/vonage/client/auth/RequestSigning.java @@ -19,8 +19,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.vonage.client.VonageUnexpectedException; import com.vonage.client.auth.hashutils.HashUtil; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.apache.http.NameValuePair; import java.io.IOException; import java.io.InputStream; @@ -28,20 +26,21 @@ import java.security.MessageDigest; import java.time.Instant; import java.util.*; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; /** * A helper class for generating or verifying MD5 signatures when signing REST requests for submission to Vonage. */ public class RequestSigning { - public static final int MAX_ALLOWABLE_TIME_DELTA = 5 * 60 * 1000; + private static final Logger LOGGER = Logger.getLogger(RequestSigning.class.getName()); + public static final int MAX_ALLOWABLE_TIME_DELTA = 5 * 60 * 1000; public static final String PARAM_SIGNATURE = "sig"; public static final String PARAM_TIMESTAMP = "timestamp"; public static final String APPLICATION_JSON = "application/json"; - private static final Log log = LogFactory.getLog(RequestSigning.class); - /** * Signs a set of request parameters. *

@@ -162,11 +161,11 @@ static Map constructSignatureForRequestParameters(Map outputParams = new LinkedHashMap<>(4); outputParams.put(PARAM_TIMESTAMP, timestampStr); @@ -242,7 +241,7 @@ static boolean verifyRequestSignature(String contentType, for (Map.Entry entry : params.entrySet()) { String name = entry.getKey(); String value = entry.getValue(); - log.info(name + " = " + value); + LOGGER.info(name + " = " + value); if (value == null || value.trim().isEmpty()) { continue; } @@ -257,7 +256,7 @@ static boolean verifyRequestSignature(String contentType, for (Map.Entry entry : parameterMap.entrySet()) { String name = entry.getKey(); String value = entry.getValue()[0]; - log.info(name + " = " + value); + LOGGER.info(name + " = " + value); if (value == null || value.trim().isEmpty()) { continue; } @@ -276,12 +275,14 @@ static boolean verifyRequestSignature(String contentType, if (timeString != null) time = Long.parseLong(timeString) * 1000; } catch (NumberFormatException e) { - log.error("Error parsing 'time' parameter [ " + timeString + " ]", e); + LOGGER.log(Level.WARNING, "Error parsing 'time' parameter [ " + timeString + " ]", e); time = 0; } long diff = currentTimeMillis - time; if (diff > MAX_ALLOWABLE_TIME_DELTA || diff < -MAX_ALLOWABLE_TIME_DELTA) { - log.warn("SECURITY-KEY-VERIFICATION -- BAD-TIMESTAMP ... Timestamp [ " + time + " ] delta [ " + diff + " ] max allowed delta [ " + -MAX_ALLOWABLE_TIME_DELTA + " ] "); + LOGGER.log(Level.WARNING, "SECURITY-KEY-VERIFICATION -- BAD-TIMESTAMP ... Timestamp [ " + + time + " ] delta [ " + diff + " ] max allowed delta [ " + -MAX_ALLOWABLE_TIME_DELTA + " ] " + ); return false; } @@ -300,12 +301,15 @@ static boolean verifyRequestSignature(String contentType, String hashed; try { hashed = HashUtil.calculate(str, secretKey, "UTF-8", hashType); - } catch (Exception e) { - log.error("error...", e); + } + catch (Exception ex) { + LOGGER.log(Level.WARNING, "error...", ex); return false; } - log.info("SECURITY-KEY-VERIFICATION -- String [ " + str + " ] Signature [ " + hashed + " ] SUPPLIED SIGNATURE [ " + suppliedSignature + " ] "); + LOGGER.info("SECURITY-KEY-VERIFICATION -- String [ " + str + " ] Signature [ " + hashed + + " ] SUPPLIED SIGNATURE [ " + suppliedSignature + " ] " + ); // verify that the supplied signature matches generated one // use MessageDigest.isEqual as an alternative to String.equals() to defend against timing based attacks diff --git a/src/test/java/com/vonage/client/incoming/InputEventTest.java b/src/test/java/com/vonage/client/incoming/InputEventTest.java index bcd76ad7a..b2a708dae 100644 --- a/src/test/java/com/vonage/client/incoming/InputEventTest.java +++ b/src/test/java/com/vonage/client/incoming/InputEventTest.java @@ -18,24 +18,23 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.junit.jupiter.api.*; import java.util.Calendar; import java.util.GregorianCalendar; import java.util.TimeZone; +import java.util.logging.Logger; import static org.junit.jupiter.api.Assertions.*; public class InputEventTest { - private static final Log LOG = LogFactory.getLog(InputEventTest.class); + private static final Logger LOG = Logger.getLogger(InputEventTest.class.getName()); @Test public void testDeserializeInputEvent() { String inputJsonStr = getInputEventJsonString().toString(); - LOG.debug(inputJsonStr); + LOG.info(inputJsonStr); - InputEvent inputEvent = InputEvent.fromJson(inputJsonStr); + var inputEvent = InputEvent.fromJson(inputJsonStr); assertEquals("aaaaaaaa-bbbb-cccc-dddd-0123456789ab", inputEvent.getUuid()); assertEquals("bbbbbbbb-cccc-dddd-eeee-0123456789ab", inputEvent.getConversationUuid()); assertTrue(inputEvent.getDtmf().isTimedOut()); @@ -65,11 +64,10 @@ public void testDeserializeInputEventSpeechError() { speechNode.put("error", expectedMessage); jsonNode.set("speech", speechNode); - InputEvent inputEvent = InputEvent.fromJson(jsonNode.toString()); + var inputEvent = InputEvent.fromJson(jsonNode.toString()); String actualMessage = inputEvent.getSpeech().getError(); assertEquals(expectedMessage, actualMessage); - } From 77da52bf8f474c86bbd59d62aaf23a5dff2aae76 Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Wed, 8 Jan 2025 18:50:40 +0000 Subject: [PATCH 2/8] Fix NPE --- .../com/vonage/client/AbstractMethod.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/vonage/client/AbstractMethod.java b/src/main/java/com/vonage/client/AbstractMethod.java index f7c9164cc..4ee4a57ba 100644 --- a/src/main/java/com/vonage/client/AbstractMethod.java +++ b/src/main/java/com/vonage/client/AbstractMethod.java @@ -16,6 +16,7 @@ package com.vonage.client; import com.vonage.client.auth.*; +import org.apache.http.Header; import org.apache.http.HttpHeaders; import org.apache.http.HttpResponse; import org.apache.http.client.methods.CloseableHttpResponse; @@ -81,11 +82,14 @@ public ResultT execute(RequestT request) throws VonageResponseParseException, Vo if (shouldLog()) { LOGGER.info("Request " + httpRequest.getMethod() + " " + httpRequest.getURI()); - StringBuilder headers = new StringBuilder("Request headers:\n"); - for (org.apache.http.Header header : httpRequest.getAllHeaders()) { - headers.append('\n').append(header.getName()).append(": ").append(header.getValue()); + Header[] headers = httpRequest.getAllHeaders(); + if (headers != null && headers.length > 0) { + StringBuilder headersStr = new StringBuilder("--- REQUEST HEADERS ---\n"); + for (Header header : headers) { + headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); + } + LOGGER.info(headersStr.toString()); } - LOGGER.info(headers.toString()); LOGGER.info("Request body: " + request); } @@ -93,11 +97,14 @@ public ResultT execute(RequestT request) throws VonageResponseParseException, Vo try { if (shouldLog()) { LOGGER.info("Response " + response.getStatusLine()); - StringBuilder headers = new StringBuilder("Response headers:\n"); - for (org.apache.http.Header header : response.getAllHeaders()) { - headers.append('\n').append(header.getName()).append(": ").append(header.getValue()); + Header[] headers = response.getAllHeaders(); + if (headers != null && headers.length > 0) { + StringBuilder headersStr = new StringBuilder("Response headers:\n"); + for (Header header : headers) { + headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); + } + LOGGER.info(headersStr.toString()); } - LOGGER.info(headers.toString()); } ResultT responseBody = parseResponse(response); From aa5d1af94001eb276d9419b243b82d3c13f17889 Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 10:53:10 +0000 Subject: [PATCH 3/8] refactor: Tidy up AbstractMethod docs / logs --- .../com/vonage/client/AbstractMethod.java | 99 ++++++++++++------- .../client/DynamicEndpointTestSpec.java | 6 +- 2 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/vonage/client/AbstractMethod.java b/src/main/java/com/vonage/client/AbstractMethod.java index 4ee4a57ba..2709e8cd2 100644 --- a/src/main/java/com/vonage/client/AbstractMethod.java +++ b/src/main/java/com/vonage/client/AbstractMethod.java @@ -39,67 +39,91 @@ *

* The REST call is executed by calling {@link #execute(Object)}. * - * @param The request object type that will be used to construct the HTTP request body. - * @param The response object type which will be constructed from the returned HTTP response body. + * @param The request object type that will be used to construct the HTTP request body. + * @param The response object type which will be constructed from the returned HTTP response body. * * @see DynamicEndpoint for an abstract implementation which handles the most common use cases. */ -public abstract class AbstractMethod implements RestEndpoint { +public abstract class AbstractMethod implements RestEndpoint { private static final Logger LOGGER = Logger.getLogger(AbstractMethod.class.getName()); private static boolean shouldLog() { return LOGGER.isLoggable(Level.INFO); } - protected final HttpWrapper httpWrapper; + private final HttpWrapper httpWrapper; - public AbstractMethod(HttpWrapper httpWrapper) { + /** + * Construct a new AbstractMethod instance with the given HTTP client. + * + * @param httpWrapper The wrapper containing the HTTP client and configuration. + */ + protected AbstractMethod(HttpWrapper httpWrapper) { this.httpWrapper = httpWrapper; } + /** + * Gets the underlying HTTP client wrapper. + * + * @return The {@link HttpWrapper} used by this endpoint. + */ public HttpWrapper getHttpWrapper() { return httpWrapper; } - protected ResultT postProcessParsedResponse(ResultT response) { + /** + * Method which allows further modification of the response object after it has been parsed. + * + * @param response The unmarshalled response object. + * + * @return The final result object to return; usually the same object that was passed in. + */ + protected RES postProcessParsedResponse(RES response) { return response; } + private HttpUriRequest createFullHttpRequest(REQ request) throws VonageClientException { + return applyAuth(makeRequest(request)) + .setHeader(HttpHeaders.USER_AGENT, httpWrapper.getUserAgent()) + .setCharset(StandardCharsets.UTF_8).build(); + } + /** - * Execute the REST call represented by this method object. + * Executes the REST call represented by this endpoint. * - * @param request A RequestT representing input to the REST call to be made + * @param request The request object representing input to the REST call to be made. * - * @return A ResultT representing the response from the executed REST call + * @return The result object representing the response from the executed REST call. * - * @throws VonageClientException if there is a problem parsing the HTTP response + * @throws VonageResponseParseException if there was a problem parsing the HTTP response. + * @throws VonageMethodFailedException if there was a problem executing the HTTP request. */ @Override - public ResultT execute(RequestT request) throws VonageResponseParseException, VonageClientException { - HttpUriRequest httpRequest = applyAuth(makeRequest(request)) - .setHeader(HttpHeaders.USER_AGENT, httpWrapper.getUserAgent()) - .setCharset(StandardCharsets.UTF_8).build(); + public RES execute(REQ request) throws VonageMethodFailedException, VonageResponseParseException { + final HttpUriRequest httpRequest = createFullHttpRequest(request); if (shouldLog()) { LOGGER.info("Request " + httpRequest.getMethod() + " " + httpRequest.getURI()); Header[] headers = httpRequest.getAllHeaders(); if (headers != null && headers.length > 0) { - StringBuilder headersStr = new StringBuilder("--- REQUEST HEADERS ---\n"); + StringBuilder headersStr = new StringBuilder("--- REQUEST HEADERS ---"); for (Header header : headers) { headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); } LOGGER.info(headersStr.toString()); } - LOGGER.info("Request body: " + request); + if (request != null) { + LOGGER.info("--- REQUEST BODY ---\n" + request); + } } - try (CloseableHttpResponse response = httpWrapper.getHttpClient().execute(httpRequest)) { + try (final CloseableHttpResponse response = httpWrapper.getHttpClient().execute(httpRequest)) { try { if (shouldLog()) { LOGGER.info("Response " + response.getStatusLine()); Header[] headers = response.getAllHeaders(); if (headers != null && headers.length > 0) { - StringBuilder headersStr = new StringBuilder("Response headers:\n"); + StringBuilder headersStr = new StringBuilder("--- RESPONSE HEADERS ---"); for (Header header : headers) { headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); } @@ -107,9 +131,9 @@ public ResultT execute(RequestT request) throws VonageResponseParseException, Vo } } - ResultT responseBody = parseResponse(response); - if (shouldLog()) { - LOGGER.info("Response body: " + responseBody); + final RES responseBody = parseResponse(response); + if (responseBody != null && shouldLog()) { + LOGGER.info("--- RESPONSE BODY ---\n" + responseBody); } return postProcessParsedResponse(responseBody); @@ -126,15 +150,15 @@ public ResultT execute(RequestT request) throws VonageResponseParseException, Vo } /** - * Apply an appropriate authentication method (specified by {@link #getAcceptableAuthMethods()}) to the provided - * {@link RequestBuilder}, and return the result. + * Apply an appropriate authentication method (specified by {@link #getAcceptableAuthMethods()}) to the + * provided {@link RequestBuilder}, and return the result. * - * @param request A RequestBuilder which has not yet had authentication information applied + * @param request A RequestBuilder which has not yet had authentication information applied. * - * @return A RequestBuilder with appropriate authentication information applied (may or not be the same instance as - *

request
) + * @return A RequestBuilder with appropriate authentication information applied + * (may or not be the same instance as
request
). * - * @throws VonageClientException If no appropriate {@link AuthMethod} is available + * @throws VonageClientException If no appropriate {@link AuthMethod} is available. */ final RequestBuilder applyAuth(RequestBuilder request) throws VonageClientException { AuthMethod am = getAuthMethod(); @@ -164,25 +188,30 @@ protected AuthMethod getAuthMethod() throws VonageUnexpectedException { return httpWrapper.getAuthCollection().getAcceptableAuthMethod(getAcceptableAuthMethods()); } + /** + * Gets applicable authentication methods for this endpoint. + * + * @return The set of acceptable authentication method classes (at least one must be provided). + */ protected abstract Set> getAcceptableAuthMethods(); /** * Construct and return a RequestBuilder instance from the provided request. * - * @param request A RequestT representing input to the REST call to be made + * @param request A request object representing input to the REST call to be made. * - * @return A ResultT representing the response from the executed REST call + * @return A RequestBuilder instance representing the HTTP request to be made. */ - protected abstract RequestBuilder makeRequest(RequestT request); + protected abstract RequestBuilder makeRequest(REQ request); /** - * Construct a ResultT representing the contents of the HTTP response returned from the Vonage Voice API. + * Construct a response object representing the contents of the HTTP response returned from the Vonage API. * - * @param response An HttpResponse returned from the Vonage Voice API + * @param response An HttpResponse returned from the Vonage API. * - * @return A ResultT type representing the result of the REST call + * @return The unmarshalled result of the REST call. * - * @throws IOException if a problem occurs parsing the response + * @throws IOException if a problem occurs parsing the response. */ - protected abstract ResultT parseResponse(HttpResponse response) throws IOException; + protected abstract RES parseResponse(HttpResponse response) throws IOException; } diff --git a/src/test/java/com/vonage/client/DynamicEndpointTestSpec.java b/src/test/java/com/vonage/client/DynamicEndpointTestSpec.java index 5e63e2faf..de88a6445 100644 --- a/src/test/java/com/vonage/client/DynamicEndpointTestSpec.java +++ b/src/test/java/com/vonage/client/DynamicEndpointTestSpec.java @@ -194,16 +194,16 @@ private void assertRequestUriAndBody(T request, String expectedRequestBodyString assertEquals(expectedUri, builder.build().getURI().toString().split("\\?")[0]); AbstractMethod endpoint = endpointAsAbstractMethod(); - HttpConfig originalConfig = endpoint.httpWrapper.getHttpConfig(); + HttpConfig originalConfig = endpoint.getHttpWrapper().getHttpConfig(); try { String baseUri = customBaseUri(); - endpoint.httpWrapper.setHttpConfig(HttpConfig.builder().baseUri(baseUri).build()); + endpoint.getHttpWrapper().setHttpConfig(HttpConfig.builder().baseUri(baseUri).build()); builder = makeTestRequest(request); expectedUri = expectedCustomBaseUri() + expectedEndpointUri(request); assertEquals(expectedUri, builder.build().getURI().toString().split("\\?")[0]); } finally { - endpoint.httpWrapper.setHttpConfig(originalConfig); + endpoint.getHttpWrapper().setHttpConfig(originalConfig); } } From a6194863ba2ac91de1b74119e3788bd5835bb6cd Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 11:36:08 +0000 Subject: [PATCH 4/8] fix: Set AM log level to FINE --- .../java/com/vonage/client/AbstractMethod.java | 15 ++++++++------- .../com/vonage/client/AbstractMethodTest.java | 6 ++++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/vonage/client/AbstractMethod.java b/src/main/java/com/vonage/client/AbstractMethod.java index 2709e8cd2..54f84aec7 100644 --- a/src/main/java/com/vonage/client/AbstractMethod.java +++ b/src/main/java/com/vonage/client/AbstractMethod.java @@ -46,9 +46,10 @@ */ public abstract class AbstractMethod implements RestEndpoint { private static final Logger LOGGER = Logger.getLogger(AbstractMethod.class.getName()); + private static final Level LOG_LEVEL = Level.FINE; private static boolean shouldLog() { - return LOGGER.isLoggable(Level.INFO); + return LOGGER.isLoggable(LOG_LEVEL); } private final HttpWrapper httpWrapper; @@ -103,37 +104,37 @@ public RES execute(REQ request) throws VonageMethodFailedException, VonageRespon final HttpUriRequest httpRequest = createFullHttpRequest(request); if (shouldLog()) { - LOGGER.info("Request " + httpRequest.getMethod() + " " + httpRequest.getURI()); + LOGGER.log(LOG_LEVEL, "Request " + httpRequest.getMethod() + " " + httpRequest.getURI()); Header[] headers = httpRequest.getAllHeaders(); if (headers != null && headers.length > 0) { StringBuilder headersStr = new StringBuilder("--- REQUEST HEADERS ---"); for (Header header : headers) { headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); } - LOGGER.info(headersStr.toString()); + LOGGER.log(LOG_LEVEL, headersStr.toString()); } if (request != null) { - LOGGER.info("--- REQUEST BODY ---\n" + request); + LOGGER.log(LOG_LEVEL, "--- REQUEST BODY ---\n" + request); } } try (final CloseableHttpResponse response = httpWrapper.getHttpClient().execute(httpRequest)) { try { if (shouldLog()) { - LOGGER.info("Response " + response.getStatusLine()); + LOGGER.log(LOG_LEVEL, "Response " + response.getStatusLine()); Header[] headers = response.getAllHeaders(); if (headers != null && headers.length > 0) { StringBuilder headersStr = new StringBuilder("--- RESPONSE HEADERS ---"); for (Header header : headers) { headersStr.append('\n').append(header.getName()).append(": ").append(header.getValue()); } - LOGGER.info(headersStr.toString()); + LOGGER.log(LOG_LEVEL, headersStr.toString()); } } final RES responseBody = parseResponse(response); if (responseBody != null && shouldLog()) { - LOGGER.info("--- RESPONSE BODY ---\n" + responseBody); + LOGGER.log(LOG_LEVEL, "--- RESPONSE BODY ---\n" + responseBody); } return postProcessParsedResponse(responseBody); diff --git a/src/test/java/com/vonage/client/AbstractMethodTest.java b/src/test/java/com/vonage/client/AbstractMethodTest.java index e41c606e2..53000e9a8 100644 --- a/src/test/java/com/vonage/client/AbstractMethodTest.java +++ b/src/test/java/com/vonage/client/AbstractMethodTest.java @@ -42,10 +42,16 @@ import java.util.Scanner; import java.util.Set; import java.util.concurrent.Executors; +import java.util.logging.LogManager; import java.util.stream.Collectors; public class AbstractMethodTest { + static { + LogManager.getLogManager().getLogger(AbstractMethod.class.getName()) + .setLevel(java.util.logging.Level.FINE); + } + private static class ConcreteMethod extends AbstractMethod { public ConcreteMethod(HttpWrapper httpWrapper) { super(httpWrapper); From 42b9afbe7d272c2f91723a9ae94c4f81a1de2e61 Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 11:44:14 +0000 Subject: [PATCH 5/8] test: Attempt to fix class init --- src/main/java/com/vonage/client/AbstractMethod.java | 5 ++++- src/test/java/com/vonage/client/AbstractMethodTest.java | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/vonage/client/AbstractMethod.java b/src/main/java/com/vonage/client/AbstractMethod.java index 54f84aec7..7a7bd980a 100644 --- a/src/main/java/com/vonage/client/AbstractMethod.java +++ b/src/main/java/com/vonage/client/AbstractMethod.java @@ -42,7 +42,7 @@ * @param The request object type that will be used to construct the HTTP request body. * @param The response object type which will be constructed from the returned HTTP response body. * - * @see DynamicEndpoint for an abstract implementation which handles the most common use cases. + * @see DynamicEndpoint for a flexible implementation which handles the most common use cases. */ public abstract class AbstractMethod implements RestEndpoint { private static final Logger LOGGER = Logger.getLogger(AbstractMethod.class.getName()); @@ -52,6 +52,9 @@ private static boolean shouldLog() { return LOGGER.isLoggable(LOG_LEVEL); } + /** + * HTTP client and configuration used by this endpoint. + */ private final HttpWrapper httpWrapper; /** diff --git a/src/test/java/com/vonage/client/AbstractMethodTest.java b/src/test/java/com/vonage/client/AbstractMethodTest.java index 53000e9a8..29708a071 100644 --- a/src/test/java/com/vonage/client/AbstractMethodTest.java +++ b/src/test/java/com/vonage/client/AbstractMethodTest.java @@ -32,6 +32,8 @@ import org.apache.http.message.BasicStatusLine; import static org.junit.jupiter.api.Assertions.*; import org.junit.jupiter.api.*; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; import org.mockito.ArgumentCaptor; import static org.mockito.Mockito.*; import java.io.*; @@ -45,6 +47,7 @@ import java.util.logging.LogManager; import java.util.stream.Collectors; +@Execution(ExecutionMode.SAME_THREAD) public class AbstractMethodTest { static { From 82b66e216f8b0cd432cfbb116dbf4244b2b77c6c Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 14:55:21 +0000 Subject: [PATCH 6/8] fix: Add logging to DynamicEndpoint --- .../com/vonage/client/DynamicEndpoint.java | 22 ++++++++++++++++--- .../com/vonage/client/AbstractMethodTest.java | 12 +++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/vonage/client/DynamicEndpoint.java b/src/main/java/com/vonage/client/DynamicEndpoint.java index 937e90aac..dc0c14fbb 100644 --- a/src/main/java/com/vonage/client/DynamicEndpoint.java +++ b/src/main/java/com/vonage/client/DynamicEndpoint.java @@ -30,6 +30,8 @@ import java.util.*; import java.util.function.BiFunction; import java.util.function.Consumer; +import java.util.logging.Level; +import java.util.logging.Logger; /** * Enables convenient declaration of endpoints without directly implementing {@link AbstractMethod}. @@ -42,6 +44,8 @@ */ @SuppressWarnings("unchecked") public class DynamicEndpoint extends AbstractMethod { + protected final Logger logger = Logger.getLogger(getClass().getName()); + protected Set> authMethods; protected String contentType, accept; protected HttpMethod requestMethod; @@ -242,6 +246,7 @@ else if (requestBody instanceof byte[]) { @Override protected final R parseResponse(HttpResponse response) throws IOException { int statusCode = response.getStatusLine().getStatusCode(); + logger.fine(() -> "Response status: " + statusCode); try { if (statusCode >= 200 && statusCode < 300) { return parseResponseSuccess(response); @@ -255,6 +260,7 @@ else if (statusCode >= 300 && statusCode < 400) { } catch (InvocationTargetException ex) { Throwable wrapped = ex.getTargetException(); + logger.log(Level.SEVERE, "Internal SDK error", ex); if (wrapped instanceof RuntimeException) { throw (RuntimeException) wrapped; } @@ -263,6 +269,7 @@ else if (statusCode >= 300 && statusCode < 400) { } } catch (ReflectiveOperationException ex) { + logger.log(Level.SEVERE, "Internal SDK error", ex); throw new VonageUnexpectedException(ex); } finally { @@ -276,6 +283,7 @@ protected R parseResponseFromString(String response) { private R parseResponseRedirect(HttpResponse response) throws ReflectiveOperationException, IOException { final String location = response.getFirstHeader("Location").getValue(); + logger.fine(() -> "Redirect: " + location); if (java.net.URI.class.equals(responseType)) { return (R) URI.create(location); @@ -290,13 +298,17 @@ else if (String.class.equals(responseType)) { private R parseResponseSuccess(HttpResponse response) throws IOException, ReflectiveOperationException { if (Void.class.equals(responseType)) { + logger.fine(() -> "No response body."); return null; } else if (byte[].class.equals(responseType)) { - return (R) EntityUtils.toByteArray(response.getEntity()); + byte[] result = EntityUtils.toByteArray(response.getEntity()); + logger.fine(() -> "Binary response body of length " + result.length); + return (R) result; } else { String deser = EntityUtils.toString(response.getEntity()); + logger.fine(() -> deser); if (responseType.equals(String.class)) { return (R) deser; @@ -316,7 +328,9 @@ else if (Collection.class.isAssignableFrom(responseType) || isJsonableArrayRespo else { R customParsedResponse = parseResponseFromString(deser); if (customParsedResponse == null) { - throw new IllegalStateException("Unhandled return type: " + responseType); + String errorMsg = "Unhandled return type: " + responseType; + logger.warning(errorMsg); + throw new IllegalStateException(errorMsg); } else { return customParsedResponse; @@ -345,7 +359,9 @@ private R parseResponseFailure(HttpResponse response) throws IOException, Reflec if (!constructor.isAccessible()) { constructor.setAccessible(true); } - throw (RuntimeException) constructor.newInstance(exMessage); + RuntimeException ex = (RuntimeException) constructor.newInstance(exMessage); + logger.log(Level.SEVERE, "Internal SDK error", ex); + throw ex; } } } diff --git a/src/test/java/com/vonage/client/AbstractMethodTest.java b/src/test/java/com/vonage/client/AbstractMethodTest.java index 29708a071..144d78773 100644 --- a/src/test/java/com/vonage/client/AbstractMethodTest.java +++ b/src/test/java/com/vonage/client/AbstractMethodTest.java @@ -47,14 +47,8 @@ import java.util.logging.LogManager; import java.util.stream.Collectors; -@Execution(ExecutionMode.SAME_THREAD) public class AbstractMethodTest { - static { - LogManager.getLogManager().getLogger(AbstractMethod.class.getName()) - .setLevel(java.util.logging.Level.FINE); - } - private static class ConcreteMethod extends AbstractMethod { public ConcreteMethod(HttpWrapper httpWrapper) { super(httpWrapper); @@ -116,6 +110,12 @@ public void close() { private AuthMethod mockAuthMethod; private final CloseableHttpResponse basicResponse = new CloseableBasicHttpResponse(); + @BeforeAll + public static void setUpBeforeClass() { + LogManager.getLogManager().getLogger(AbstractMethod.class.getName()) + .setLevel(java.util.logging.Level.FINE); + } + @BeforeEach public void setUp() throws Exception { mockWrapper = mock(HttpWrapper.class); From b03d43de748a3671d02dd12f82142ccb76c51a80 Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 16:24:56 +0000 Subject: [PATCH 7/8] test: Fix class initialisation bug --- .../java/com/vonage/client/AbstractMethodTest.java | 13 +++++-------- .../java/com/vonage/client/DynamicEndpointTest.java | 6 +++++- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/test/java/com/vonage/client/AbstractMethodTest.java b/src/test/java/com/vonage/client/AbstractMethodTest.java index 144d78773..1d5031cae 100644 --- a/src/test/java/com/vonage/client/AbstractMethodTest.java +++ b/src/test/java/com/vonage/client/AbstractMethodTest.java @@ -32,8 +32,6 @@ import org.apache.http.message.BasicStatusLine; import static org.junit.jupiter.api.Assertions.*; import org.junit.jupiter.api.*; -import org.junit.jupiter.api.parallel.Execution; -import org.junit.jupiter.api.parallel.ExecutionMode; import org.mockito.ArgumentCaptor; import static org.mockito.Mockito.*; import java.io.*; @@ -54,6 +52,11 @@ public ConcreteMethod(HttpWrapper httpWrapper) { super(httpWrapper); } + static { + LogManager.getLogManager().getLogger(AbstractMethod.class.getName()) + .setLevel(java.util.logging.Level.FINE); + } + RequestBuilder makeRequest() throws UnsupportedEncodingException { return makeRequest("http://example.org/resource") .addParameter("foo", "bar") @@ -110,12 +113,6 @@ public void close() { private AuthMethod mockAuthMethod; private final CloseableHttpResponse basicResponse = new CloseableBasicHttpResponse(); - @BeforeAll - public static void setUpBeforeClass() { - LogManager.getLogManager().getLogger(AbstractMethod.class.getName()) - .setLevel(java.util.logging.Level.FINE); - } - @BeforeEach public void setUp() throws Exception { mockWrapper = mock(HttpWrapper.class); diff --git a/src/test/java/com/vonage/client/DynamicEndpointTest.java b/src/test/java/com/vonage/client/DynamicEndpointTest.java index 6cf59f7d4..02fb92811 100644 --- a/src/test/java/com/vonage/client/DynamicEndpointTest.java +++ b/src/test/java/com/vonage/client/DynamicEndpointTest.java @@ -22,16 +22,20 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import java.net.URI; +import java.util.logging.Level; +import java.util.logging.Logger; public class DynamicEndpointTest { private static final HttpWrapper WRAPPER = new HttpWrapper(new NoAuthMethod()); @SuppressWarnings("unchecked") static DynamicEndpoint newEndpoint(R... responseType) { - return DynamicEndpoint. builder(responseType) + var endpoint = DynamicEndpoint. builder(responseType) .wrapper(WRAPPER).authMethod(NoAuthMethod.class) .pathGetter((de, req) -> TEST_BASE_URI) .requestMethod(HttpMethod.GET).acceptHeader("text").build(); + Logger.getLogger(endpoint.getClass().getName()).setLevel(Level.FINE); + return endpoint; } @Test From ab7f3a8b1ee227001a46698385a4e4b2df6ae52a Mon Sep 17 00:00:00 2001 From: Sina Madani Date: Thu, 9 Jan 2025 16:34:26 +0000 Subject: [PATCH 8/8] Add missing logs in DynamicEndpoint --- src/main/java/com/vonage/client/DynamicEndpoint.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/vonage/client/DynamicEndpoint.java b/src/main/java/com/vonage/client/DynamicEndpoint.java index dc0c14fbb..d4f701911 100644 --- a/src/main/java/com/vonage/client/DynamicEndpoint.java +++ b/src/main/java/com/vonage/client/DynamicEndpoint.java @@ -329,7 +329,7 @@ else if (Collection.class.isAssignableFrom(responseType) || isJsonableArrayRespo R customParsedResponse = parseResponseFromString(deser); if (customParsedResponse == null) { String errorMsg = "Unhandled return type: " + responseType; - logger.warning(errorMsg); + logger.severe(errorMsg); throw new IllegalStateException(errorMsg); } else { @@ -350,6 +350,7 @@ private R parseResponseFailure(HttpResponse response) throws IOException, Reflec varex.title = response.getStatusLine().getReasonPhrase(); } varex.statusCode = response.getStatusLine().getStatusCode(); + logger.log(Level.WARNING, "Failed to parse response", varex); throw varex; } else { @@ -368,6 +369,7 @@ private R parseResponseFailure(HttpResponse response) throws IOException, Reflec } R customParsedResponse = parseResponseFromString(exMessage); if (customParsedResponse == null) { + logger.warning(exMessage); throw new VonageApiResponseException(exMessage); } else {