tweaks

Author: schlosna

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java

@@ -73,7 +73,6 @@ final class DialogueFeignClient implements feign.Client {
     private static final Splitter PATH_SPLITTER = Splitter.on('/');
     private static final Splitter QUERY_SPLITTER = Splitter.on('&').omitEmptyStrings();
     private static final char QUERY_KEY_VALUE_SEPARATOR = '=';
-    private static final Splitter QUERY_KEY_VALUE_SPLITTER = Splitter.on(QUERY_KEY_VALUE_SEPARATOR);
 
     private final ConjureRuntime runtime;
     private final Channel channel;
@@ -419,23 +418,18 @@ public void renderPath(ListMultimap<String, String> params, UrlBuilder url) {
             if (queryParamsStart != -1) {
                 String querySegments = trailing.substring(queryParamsStart + 1);
                 for (String querySegment : QUERY_SPLITTER.split(querySegments)) {
-                    boolean isValid = false;
                     int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR);
-                    if (equalsIndex > -1) {
+                    if (equalsIndex > 0) {
                         String key = querySegment.substring(0, equalsIndex);
-                        int valueStart = equalsIndex + 1;
-                        if (querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR, valueStart) == -1) {
-                            isValid = true;
-                            url.queryParam(urlDecode(key), urlDecode(querySegment.substring(valueStart)));
-                        }
-                    }
-
-                    if (!isValid) {
-                        List<String> keyValuePair = QUERY_KEY_VALUE_SPLITTER.splitToList(querySegment);
+                        String value = querySegment.substring(equalsIndex + 1);
+                        url.queryParam(urlDecode(key), urlDecode(value));
+                    } else {
                         throw new SafeIllegalStateException(
                                 "Expected two parameters",
-                                SafeArg.of("parameters", keyValuePair.size()),
-                                UnsafeArg.of("values", keyValuePair));
+                                SafeArg.of(
+                                        "parameters",
+                                        querySegment.split(String.valueOf(QUERY_KEY_VALUE_SEPARATOR)).length),
+                                UnsafeArg.of("values", querySegment));
                     }
                 }
             }

conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java

@@ -16,10 +16,13 @@
 
 package com.palantir.conjure.java.client.jaxrs;
 
+import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.InstanceOfAssertFactories.collection;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -35,6 +38,9 @@
 import com.palantir.dialogue.Request;
 import com.palantir.dialogue.Response;
 import com.palantir.dialogue.UrlBuilder;
+import com.palantir.logsafe.SafeArg;
+import com.palantir.logsafe.UnsafeArg;
+import com.palantir.logsafe.exceptions.SafeIllegalStateException;
 import jakarta.ws.rs.Consumes;
 import jakarta.ws.rs.GET;
 import jakarta.ws.rs.POST;
@@ -95,6 +101,56 @@ public void testQueryParameterCollection() {
         verify(urlBuilder).queryParam("query", "a+b");
     }
 
+    @Test
+    public void testQueryParameters() {
+        Channel channel = stubNoContentResponseChannel();
+        StubService service = JaxRsClient.create(StubService.class, channel, runtime);
+        service.collectionOfQueryParams(List.of("", "=", "test=value"));
+
+        ArgumentCaptor<Endpoint> endpointCaptor = ArgumentCaptor.forClass(Endpoint.class);
+        ArgumentCaptor<Request> requestCaptor = ArgumentCaptor.forClass(Request.class);
+        verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture());
+        assertThat(requestCaptor.getValue().pathParameters().asMap())
+                .hasSize(1)
+                .extractingByKey("request-url")
+                .asInstanceOf(collection(String.class))
+                .containsExactly("dialogue://feign/foo/params?query=&query=%3D&query=test%3Dvalue");
+        Endpoint endpoint = endpointCaptor.getValue();
+        UrlBuilder urlBuilder = mock(UrlBuilder.class);
+        endpoint.renderPath(
+                ImmutableListMultimap.of("request-url", "dialogue://feign/foo/params?query=&query==&query=test=value"),
+                urlBuilder);
+        verify(urlBuilder).queryParam("query", "");
+        verify(urlBuilder).queryParam("query", "=");
+        verify(urlBuilder).queryParam("query", "test=value");
+    }
+
+    @Test
+    public void testInvalidQueryParameters() {
+        Channel channel = stubNoContentResponseChannel();
+        StubService service = JaxRsClient.create(StubService.class, channel, runtime);
+        service.collectionOfQueryParams(List.of());
+
+        ArgumentCaptor<Endpoint> endpointCaptor = ArgumentCaptor.forClass(Endpoint.class);
+        ArgumentCaptor<Request> requestCaptor = ArgumentCaptor.forClass(Request.class);
+        verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture());
+        Endpoint endpoint = endpointCaptor.getValue();
+        UrlBuilder urlBuilder = mock(UrlBuilder.class);
+        assertThatLoggableExceptionThrownBy(() -> endpoint.renderPath(
+                        ImmutableListMultimap.of("request-url", "dialogue://feign/foo/params?="), urlBuilder))
+                .isInstanceOf(SafeIllegalStateException.class)
+                .hasLogMessage("Expected two parameters")
+                .args()
+                .containsExactlyInAnyOrder(SafeArg.of("parameters", 0), UnsafeArg.of("values", "="));
+        assertThatLoggableExceptionThrownBy(() -> endpoint.renderPath(
+                        ImmutableListMultimap.of("request-url", "dialogue://feign/foo/params?query"), urlBuilder))
+                .isInstanceOf(SafeIllegalStateException.class)
+                .hasLogMessage("Expected two parameters")
+                .args()
+                .containsExactlyInAnyOrder(SafeArg.of("parameters", 1), UnsafeArg.of("values", "query"));
+        verify(urlBuilder, never()).queryParam(any(), any());
+    }
+
     @Test
     public void testPostWithBody() {
         Channel channel = stubNoContentResponseChannel();