From c68f22f0203ec4d5cb2bb63c8e7ce9bc7ce59948 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 20 Aug 2025 16:20:13 -0400 Subject: [PATCH 1/4] Optimize query parameter key/value parsing --- .../java/client/jaxrs/DialogueFeignClient.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java index de98b3118..59b8bb393 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java @@ -72,7 +72,8 @@ final class DialogueFeignClient implements feign.Client { private static final String REQUEST_URL_PATH_PARAM = "request-url"; private static final Splitter PATH_SPLITTER = Splitter.on('/'); private static final Splitter QUERY_SPLITTER = Splitter.on('&').omitEmptyStrings(); - private static final Splitter QUERY_VALUE_SPLITTER = Splitter.on('='); + 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; @@ -386,6 +387,7 @@ private final class FeignEndpoint implements Endpoint { } @Override + @SuppressWarnings("CyclomaticComplexity") public void renderPath(ListMultimap params, UrlBuilder url) { List requestUrls = params.get(REQUEST_URL_PATH_PARAM); Preconditions.checkState( @@ -417,14 +419,21 @@ public void renderPath(ListMultimap params, UrlBuilder url) { if (queryParamsStart != -1) { String querySegments = trailing.substring(queryParamsStart + 1); for (String querySegment : QUERY_SPLITTER.split(querySegments)) { - List keyValuePair = QUERY_VALUE_SPLITTER.splitToList(querySegment); - if (keyValuePair.size() != 2) { + int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR); + if (equalsIndex > -1) { + String key = querySegment.substring(0, equalsIndex); + int valueStart = equalsIndex + 1; + if (querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR, valueStart) == -1) { + String value = querySegment.substring(valueStart); + url.queryParam(urlDecode(key), urlDecode(value)); + } + } else { + List keyValuePair = QUERY_KEY_VALUE_SPLITTER.splitToList(querySegment); throw new SafeIllegalStateException( "Expected two parameters", SafeArg.of("parameters", keyValuePair.size()), UnsafeArg.of("values", keyValuePair)); } - url.queryParam(urlDecode(keyValuePair.get(0)), urlDecode(keyValuePair.get(1))); } } } From 19bb3d851bf61cdc94ee65d2e2c1092cc82c5d9d Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 20 Aug 2025 16:31:23 -0400 Subject: [PATCH 2/4] fixup --- .../conjure/java/client/jaxrs/DialogueFeignClient.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java index 59b8bb393..a5bedae0d 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java @@ -419,15 +419,18 @@ public void renderPath(ListMultimap 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) { String key = querySegment.substring(0, equalsIndex); int valueStart = equalsIndex + 1; if (querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR, valueStart) == -1) { - String value = querySegment.substring(valueStart); - url.queryParam(urlDecode(key), urlDecode(value)); + isValid = true; + url.queryParam(urlDecode(key), urlDecode(querySegment.substring(valueStart))); } - } else { + } + + if (!isValid) { List keyValuePair = QUERY_KEY_VALUE_SPLITTER.splitToList(querySegment); throw new SafeIllegalStateException( "Expected two parameters", From 53fb02625295a14e3a6d1b64fe05a42a9ab053ee Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 21 Aug 2025 10:00:48 -0400 Subject: [PATCH 3/4] tweaks --- .../client/jaxrs/DialogueFeignClient.java | 22 +++----- .../JaxRsClientDialogueEndpointTest.java | 56 +++++++++++++++++++ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java index a5bedae0d..aedf3c34e 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java +++ b/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 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 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)); } } } diff --git a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java b/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java index 1395d4463..f35090f9e 100644 --- a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java +++ b/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 endpointCaptor = ArgumentCaptor.forClass(Endpoint.class); + ArgumentCaptor 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 endpointCaptor = ArgumentCaptor.forClass(Endpoint.class); + ArgumentCaptor 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(); From f7b5e4c380826c993cdfdf94924134b5c0fed052 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 21 Aug 2025 10:16:14 -0400 Subject: [PATCH 4/4] tweak --- .../java/client/jaxrs/DialogueFeignClient.java | 4 +--- .../jaxrs/JaxRsClientDialogueEndpointTest.java | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java index aedf3c34e..54eca4768 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java @@ -426,9 +426,7 @@ public void renderPath(ListMultimap params, UrlBuilder url) { } else { throw new SafeIllegalStateException( "Expected two parameters", - SafeArg.of( - "parameters", - querySegment.split(String.valueOf(QUERY_KEY_VALUE_SEPARATOR)).length), + SafeArg.of("parameters", equalsIndex == -1 ? 0 : 1), UnsafeArg.of("values", querySegment)); } } diff --git a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java b/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java index f35090f9e..fe1bbe64f 100644 --- a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java +++ b/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java @@ -105,7 +105,7 @@ public void testQueryParameterCollection() { public void testQueryParameters() { Channel channel = stubNoContentResponseChannel(); StubService service = JaxRsClient.create(StubService.class, channel, runtime); - service.collectionOfQueryParams(List.of("", "=", "test=value")); + service.collectionOfQueryParams(List.of("", "=", "value", "test=value")); ArgumentCaptor endpointCaptor = ArgumentCaptor.forClass(Endpoint.class); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); @@ -114,11 +114,12 @@ public void testQueryParameters() { .hasSize(1) .extractingByKey("request-url") .asInstanceOf(collection(String.class)) - .containsExactly("dialogue://feign/foo/params?query=&query=%3D&query=test%3Dvalue"); + .containsExactly("dialogue://feign/foo/params?query=&query=%3D&query=value&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"), + ImmutableListMultimap.of( + "request-url", "dialogue://feign/foo/params?query=&query==&query=value&query=test=value"), urlBuilder); verify(urlBuilder).queryParam("query", ""); verify(urlBuilder).queryParam("query", "="); @@ -141,13 +142,19 @@ public void testInvalidQueryParameters() { .isInstanceOf(SafeIllegalStateException.class) .hasLogMessage("Expected two parameters") .args() - .containsExactlyInAnyOrder(SafeArg.of("parameters", 0), UnsafeArg.of("values", "=")); + .containsExactlyInAnyOrder(SafeArg.of("parameters", 1), UnsafeArg.of("values", "=")); + assertThatLoggableExceptionThrownBy(() -> endpoint.renderPath( + ImmutableListMultimap.of("request-url", "dialogue://feign/foo/params?=value"), urlBuilder)) + .isInstanceOf(SafeIllegalStateException.class) + .hasLogMessage("Expected two parameters") + .args() + .containsExactlyInAnyOrder(SafeArg.of("parameters", 1), UnsafeArg.of("values", "=value")); 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")); + .containsExactlyInAnyOrder(SafeArg.of("parameters", 0), UnsafeArg.of("values", "query")); verify(urlBuilder, never()).queryParam(any(), any()); }