Fix a number of HTTP client bugs

src/main/java/engineer/nightowl/sonos/api/SonosApiClient.java

@@ -47,6 +47,9 @@ public class SonosApiClient implements AutoCloseable
     // Can be overridden by implementing applications
     private SonosApiConfiguration configuration;
     private HttpClient httpClient;
+    // Whether this client created the HttpClient itself (and may therefore close it). A caller-supplied
+    // client is owned by the caller and must not be closed on our behalf.
+    private boolean httpClientOwned;
 
     private final String version;
 
@@ -77,8 +80,17 @@ public SonosApiClient(final SonosApiConfiguration configuration, final HttpClien
         logger.info("Initialising sonos-api-java:{}", version);
 
         this.configuration = configuration;
-        this.httpClient = (httpClient == null ? generateHttpClient() : httpClient);
-
+        if (httpClient == null)
+        {
+            this.httpClient = generateHttpClient();
+            this.httpClientOwned = true;
+        }
+        else
+        {
+            this.httpClient = httpClient;
+            this.httpClientOwned = false;
+        }
+
 
         // Setup resources
         audioClipResource = new AudioClipResource(this);
@@ -111,17 +123,31 @@ public String getUserAgent()
     private HttpClient generateHttpClient()
     {
         logger.debug("Using default HttpClient");
-        return HttpClient.newBuilder()
-                .followRedirects(HttpClient.Redirect.NORMAL)
-                .build();
+        final HttpClient.Builder builder = HttpClient.newBuilder()
+                .followRedirects(HttpClient.Redirect.NORMAL);
+        final var connectTimeout = configuration.getConnectTimeout();
+        if (connectTimeout != null && !connectTimeout.isZero() && !connectTimeout.isNegative())
+        {
+            builder.connectTimeout(connectTimeout);
+        }
+        return builder.build();
     }
 
     /**
-     * Close the HTTP client.
+     * Close the HTTP client, but only if this instance created it. A caller-supplied {@link HttpClient}
+     * is owned by the caller (it may be shared elsewhere), so it is left open; close it yourself via
+     * {@link #getHttpClient()} if required.
      */
     public void closeHttpClient()
     {
-        httpClient.close();
+        if (httpClientOwned)
+        {
+            httpClient.close();
+        }
+        else
+        {
+            logger.debug("Not closing caller-supplied HttpClient");
+        }
     }
 
     /**
@@ -142,6 +168,8 @@ public HttpClient getHttpClient()
     public void setHttpClient(final HttpClient httpClient)
     {
         this.httpClient = httpClient;
+        // A client set after construction is caller-supplied and must not be closed on their behalf.
+        this.httpClientOwned = false;
     }
 
     private void loadProperties()

src/main/java/engineer/nightowl/sonos/api/SonosApiConfiguration.java

@@ -4,6 +4,7 @@
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 
 import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.util.Base64;
 
 /**
@@ -20,6 +21,8 @@ public class SonosApiConfiguration
     private String authBaseUrl;
     private String controlBaseUrl;
     private Boolean clientSideValidationEnabled;
+    private Duration connectTimeout;
+    private Duration requestTimeout;
 
     /**
      * <p>Constructor for SonosApiConfiguration.</p>
@@ -124,12 +127,46 @@ public void setClientSideValidationEnabled(Boolean clientSideValidationEnabled)
         this.clientSideValidationEnabled = clientSideValidationEnabled;
     }
 
+    /**
+     * <p>Getter for the field <code>connectTimeout</code> - how long to wait when establishing a
+     * connection to Sonos.</p>
+     *
+     * @return the connect timeout
+     */
+    public Duration getConnectTimeout()
+    {
+        return connectTimeout;
+    }
+
+    public void setConnectTimeout(final Duration connectTimeout)
+    {
+        this.connectTimeout = connectTimeout;
+    }
+
+    /**
+     * <p>Getter for the field <code>requestTimeout</code> - how long to wait for a response before a
+     * single request is aborted.</p>
+     *
+     * @return the request timeout
+     */
+    public Duration getRequestTimeout()
+    {
+        return requestTimeout;
+    }
+
+    public void setRequestTimeout(final Duration requestTimeout)
+    {
+        this.requestTimeout = requestTimeout;
+    }
+
 
     public void loadDefaults()
     {
         setAuthBaseUrl("api.sonos.com");
         setControlBaseUrl("api.ws.sonos.com/control/api");
         setClientSideValidationEnabled(Boolean.TRUE);
+        setConnectTimeout(Duration.ofSeconds(10));
+        setRequestTimeout(Duration.ofSeconds(30));
     }
 
     /**
@@ -154,6 +191,8 @@ public String toString()
                 ", authBaseUrl='" + authBaseUrl + '\'' +
                 ", controlBaseUrl='" + controlBaseUrl + '\'' +
                 ", clientSideValidationEnabled=" + clientSideValidationEnabled +
+                ", connectTimeout=" + connectTimeout +
+                ", requestTimeout=" + requestTimeout +
                 '}';
     }
 
@@ -185,6 +224,8 @@ public boolean equals(Object o)
                 .append(authBaseUrl, that.authBaseUrl)
                 .append(controlBaseUrl, that.controlBaseUrl)
                 .append(clientSideValidationEnabled, that.clientSideValidationEnabled)
+                .append(connectTimeout, that.connectTimeout)
+                .append(requestTimeout, that.requestTimeout)
                 .isEquals();
     }
 
@@ -198,6 +239,8 @@ public int hashCode()
                 .append(authBaseUrl)
                 .append(controlBaseUrl)
                 .append(clientSideValidationEnabled)
+                .append(connectTimeout)
+                .append(requestTimeout)
                 .toHashCode();
     }
 }

src/main/java/engineer/nightowl/sonos/api/resource/AuthorizeResource.java

@@ -144,12 +144,16 @@ private HttpRequest buildOauthAccessRequest(final List<Map.Entry<String, String>
             throw new SonosApiClientException("Invalid URI built", e);
         }
 
-        return HttpRequest.newBuilder(uri)
+        final HttpRequest.Builder builder = HttpRequest.newBuilder(uri)
                 .setHeader("Authorization", configuration.getAuthorizationHeader())
                 .setHeader("Content-Type", "application/x-www-form-urlencoded; charset=UTF-8")
                 .setHeader("User-Agent", apiClient.getUserAgent())
-                .POST(HttpRequest.BodyPublishers.ofString(encodeParameters(postParameters), StandardCharsets.UTF_8))
-                .build();
+                .POST(HttpRequest.BodyPublishers.ofString(encodeParameters(postParameters), StandardCharsets.UTF_8));
+        if (isPositive(configuration.getRequestTimeout()))
+        {
+            builder.timeout(configuration.getRequestTimeout());
+        }
+        return builder.build();
     }
 
     /**

src/main/java/engineer/nightowl/sonos/api/resource/BaseResource.java

@@ -16,9 +16,11 @@
 
 import java.io.IOException;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.util.Optional;
 
 /**
@@ -69,12 +71,37 @@ <T> T callApi(final HttpRequest request, final Class<T> type) throws SonosApiCli
         final byte[] bytes = response.body();
 
         logger.debug("Raw response from API: {}", response);
-        logger.debug("Raw response content from API: {}", bytes);
+        if (logger.isDebugEnabled())
+        {
+            // A token response body contains OAuth access/refresh tokens - never write those to logs,
+            // even at debug. Other bodies are decoded as text (logging the raw byte[] would just print
+            // its identity hash, which is useless).
+            if (isTokenResponse(request))
+            {
+                logger.debug("Raw response content from API: <redacted - token response>");
+            }
+            else
+            {
+                logger.debug("Raw response content from API: {}", new String(bytes, StandardCharsets.UTF_8));
+            }
+        }
 
         // Get type from Sonos response - not always possible
         final SonosType sonosDeclaredClass = getTypeFromHeader(response);
         final String sonosDeclaredClassName = sonosDeclaredClass == null ? null : sonosDeclaredClass.getClazz().getSimpleName();
 
+        // A non-2xx response that Sonos did not describe with a structured error type would otherwise be
+        // handed to the deserializer for the requested type, producing a confusing "converting response"
+        // parse error. Surface the HTTP status (and a short body snippet) instead. Responses that DO carry
+        // a declared error type still flow through to the SonosApiError handling below, regardless of status.
+        final int status = response.statusCode();
+        final boolean sonosDeclaredError = sonosDeclaredClass != null && SonosType.getErrorTypes().contains(sonosDeclaredClass);
+        if ((status < 200 || status >= 300) && !sonosDeclaredError)
+        {
+            final String detail = isTokenResponse(request) ? "<redacted - token response>" : bodySnippet(bytes);
+            throw new SonosApiClientException(String.format("Sonos API returned HTTP %d: %s", status, detail));
+        }
+
         // If Sonos didn't provide a type, or if one was provided and it matches what we wanted returned, proceed
         if (sonosDeclaredClassName == null || sonosDeclaredClassName.equals(type.getSimpleName()))
         {
@@ -211,7 +238,10 @@ <T> T deleteFromApi(final Class<T> returnType, final String token, final String
      */
     <T> T postToApi(final Class<T> returnType, final String token, final String path) throws SonosApiClientException, SonosApiError
     {
-        return postToApi(returnType, token, path, new String[0]);
+        // Pass null (not an empty array) so isEmpty() sees "no content" and the request is sent with
+        // no body. isEmpty() does not treat a non-null array as empty, so a sentinel like new String[0]
+        // would be serialized to "[]" and sent as a bogus application/json body.
+        return postToApi(returnType, token, path, (Object) null);
     }
 
     /**
@@ -273,18 +303,42 @@ <T, U> T postToApi(final Class<T> returnType, final String token, final String p
     HttpRequest.Builder getStandardRequest(final String token, final String path) throws SonosApiClientException
     {
         final SonosApiConfiguration configuration = apiClient.getConfiguration();
-        final URI uri;
+        final URI uri = buildControlUri(configuration.getControlBaseUrl(), path);
+
+        final HttpRequest.Builder builder = HttpRequest.newBuilder(uri)
+                .setHeader("Authorization", String.format("Bearer %s", token))
+                .setHeader("User-Agent", apiClient.getUserAgent());
+        if (isPositive(configuration.getRequestTimeout()))
+        {
+            builder.timeout(configuration.getRequestTimeout());
+        }
+        return builder;
+    }
+
+    /**
+     * Build the control API URI for a path, percent-encoding the path segments (which may contain
+     * interpolated resource IDs). {@code controlBaseUrl} bundles the host with a base path, e.g.
+     * {@code api.ws.sonos.com/control/api}, so it is split on the first {@code /} to let the multi-argument
+     * {@link URI} constructor encode the assembled path - unlike {@link URI#create(String)}, which requires
+     * an already-valid URI string and would reject (rather than encode) a stray space or reserved character.
+     *
+     * @param controlBaseUrl the configured control base URL (host + optional base path)
+     * @param path           the resource path, already interpolated with any IDs
+     * @return the assembled, encoded URI
+     * @throws SonosApiClientException if the URI could not be built
+     */
+    private static URI buildControlUri(final String controlBaseUrl, final String path) throws SonosApiClientException
+    {
+        final int firstSlash = controlBaseUrl.indexOf('/');
+        final String host = firstSlash < 0 ? controlBaseUrl : controlBaseUrl.substring(0, firstSlash);
+        final String basePath = firstSlash < 0 ? "" : controlBaseUrl.substring(firstSlash);
         try
         {
-            uri = URI.create("https://" + configuration.getControlBaseUrl() + path);
-        } catch (final IllegalArgumentException e)
+            return new URI("https", host, basePath + path, null);
+        } catch (final URISyntaxException e)
         {
             throw new SonosApiClientException("Invalid URI built", e);
         }
-
-        return HttpRequest.newBuilder(uri)
-                .setHeader("Authorization", String.format("Bearer %s", token))
-                .setHeader("User-Agent", apiClient.getUserAgent());
     }
 
     /**
@@ -313,6 +367,46 @@ HttpRequest getDeleteRequest(final String token, final String path) throws Sonos
         return getStandardRequest(token, path).DELETE().build();
     }
 
+    /**
+     * Whether a request targets the OAuth token endpoint, whose response body carries access/refresh
+     * tokens that must be kept out of logs.
+     *
+     * @param request the request being made
+     * @return true if this is a token request
+     */
+    private static boolean isTokenResponse(final HttpRequest request)
+    {
+        final String path = request.uri().getPath();
+        return path != null && path.contains("/oauth/access");
+    }
+
+    /**
+     * Decode a response body as text for inclusion in an error message, truncated so an unexpectedly large
+     * body (e.g. an HTML error page from an intermediary) cannot bloat the exception.
+     *
+     * @param bytes the raw response body
+     * @return a short, human-readable snippet of the body
+     */
+    private static String bodySnippet(final byte[] bytes)
+    {
+        final String body = new String(bytes, StandardCharsets.UTF_8);
+        final int maxLength = 512;
+        return body.length() <= maxLength ? body : body.substring(0, maxLength) + "...(truncated)";
+    }
+
+    /**
+     * A timeout only applies if it is present and strictly positive - {@link HttpRequest.Builder#timeout}
+     * and {@link java.net.http.HttpClient.Builder#connectTimeout} both reject zero/negative durations, so a
+     * non-positive configured value is treated as "no timeout" rather than an error.
+     *
+     * @param timeout the configured timeout, possibly null
+     * @return true if the timeout should be applied
+     */
+    static boolean isPositive(final Duration timeout)
+    {
+        return timeout != null && !timeout.isZero() && !timeout.isNegative();
+    }
+
     void validateNotNull(final Object o) throws SonosApiClientException
     {
         validateNotNull(o, null);

src/test/java/engineer/nightowl/sonos/api/SonosApiClientTest.java

@@ -0,0 +1,44 @@
+package engineer.nightowl.sonos.api;
+
+import org.junit.jupiter.api.Test;
+
+import java.net.http.HttpClient;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+class SonosApiClientTest
+{
+    private static SonosApiConfiguration testConfiguration()
+    {
+        final SonosApiConfiguration configuration = new SonosApiConfiguration();
+        configuration.setApiKey("testApiKey");
+        configuration.setApiSecret("testApiSecret");
+        configuration.setApplicationId("test");
+        return configuration;
+    }
+
+    @Test
+    void closeDoesNotCloseCallerSuppliedClient()
+    {
+        final HttpClient suppliedClient = mock(HttpClient.class);
+        final SonosApiClient client = new SonosApiClient(testConfiguration(), suppliedClient);
+
+        client.close();
+
+        verify(suppliedClient, never()).close();
+    }
+
+    @Test
+    void closeDoesNotCloseClientSuppliedViaSetter()
+    {
+        final SonosApiClient client = new SonosApiClient(testConfiguration());
+        final HttpClient suppliedClient = mock(HttpClient.class);
+        client.setHttpClient(suppliedClient);
+
+        client.close();
+
+        verify(suppliedClient, never()).close();
+    }
+}