Improve non-200 responses, encode API path segments

Signed-off-by: James Milligan <hello@nightowl.engineer>

Author: nightowlengineer

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

@@ -16,6 +16,7 @@
 
 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;
@@ -89,6 +90,18 @@ <T> T callApi(final HttpRequest request, final Class<T> type) throws SonosApiCli
         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()))
         {
@@ -290,14 +303,7 @@ <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;
-        try
-        {
-            uri = URI.create("https://" + configuration.getControlBaseUrl() + path);
-        } catch (final IllegalArgumentException e)
-        {
-            throw new SonosApiClientException("Invalid URI built", e);
-        }
+        final URI uri = buildControlUri(configuration.getControlBaseUrl(), path);
 
         final HttpRequest.Builder builder = HttpRequest.newBuilder(uri)
                 .setHeader("Authorization", String.format("Bearer %s", token))
@@ -309,6 +315,32 @@ HttpRequest.Builder getStandardRequest(final String token, final String path) th
         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
+        {
+            return new URI("https", host, basePath + path, null);
+        } catch (final URISyntaxException e)
+        {
+            throw new SonosApiClientException("Invalid URI built", e);
+        }
+    }
+
     /**
      * Helper method to generate a basic GET request
      *
@@ -348,6 +380,20 @@ private static boolean isTokenResponse(final HttpRequest request)
         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

src/test/java/engineer/nightowl/sonos/api/resource/BaseResourceTest.java

@@ -177,6 +177,22 @@ void testApiErrorHandledCorrectly() throws Exception
         }
     }
 
+    @Test
+    void testNonSuccessStatusWithoutErrorTypeThrowsWithStatus() throws Exception
+    {
+        final byte[] bytes = "Service Unavailable".getBytes(StandardCharsets.UTF_8);
+
+        final HttpResponse<byte[]> mockedResponse = mock(HttpResponse.class);
+        when(mockedResponse.body()).thenReturn(bytes);
+        when(mockedResponse.statusCode()).thenReturn(503);
+        when(mockedResponse.headers()).thenReturn(HttpHeaders.of(Map.of(), (a, b) -> true));
+        when(mockedClient.send(any(HttpRequest.class), ArgumentMatchers.<HttpResponse.BodyHandler<byte[]>>any())).thenReturn(mockedResponse);
+
+        final SonosApiClientException e = assertThrows(SonosApiClientException.class,
+                () -> baseResource.getFromApi(SonosHomeTheaterOptions.class, "token123", "some/test"));
+        assertEquals("Sonos API returned HTTP 503: Service Unavailable", e.getMessage());
+    }
+
     @Test
     void testApiMismatchHandledCorrectly() throws Exception
     {
@@ -215,6 +231,18 @@ void getStandardRequest() throws SonosApiClientException
         assertEquals("/control/api/some/path", req.uri().getPath());
     }
 
+    @Test
+    void getStandardRequestEncodesPathSegments() throws SonosApiClientException
+    {
+        // An id containing a character that is illegal in a raw URI (here a space) must be percent-encoded
+        // rather than rejected. getPath() returns the decoded path; getRawPath() shows the encoding.
+        final HttpRequest req = baseResource.getStandardRequest("token123", "/v1/players/a b/playerVolume").GET().build();
+
+        assertEquals("api.example.com", req.uri().getHost());
+        assertEquals("/control/api/v1/players/a b/playerVolume", req.uri().getPath());
+        assertEquals("/control/api/v1/players/a%20b/playerVolume", req.uri().getRawPath());
+    }
+
     @Test
     void getStandardRequestAppliesConfiguredRequestTimeout() throws SonosApiClientException
     {