Skip to content

Optimize query parameter key=value parsing #3200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Optimize query parameter key=value parsing #3200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c68f22f
Optimize query parameter key/value parsing
schlosna Aug 20, 2025
19bb3d8
fixup
schlosna Aug 20, 2025
53fb026
tweaks
schlosna Aug 21, 2025
f7b5e4c
tweak
schlosna Aug 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions ...axrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ 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 final ConjureRuntime runtime;
private final Channel channel;
Expand Down Expand Up @@ -386,6 +386,7 @@ private final class FeignEndpoint implements Endpoint {
}

@Override
@SuppressWarnings("CyclomaticComplexity")
public void renderPath(ListMultimap<String, String> params, UrlBuilder url) {
List<String> requestUrls = params.get(REQUEST_URL_PATH_PARAM);
Preconditions.checkState(
Expand Down Expand Up @@ -417,14 +418,17 @@ 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)) {
List<String> keyValuePair = QUERY_VALUE_SPLITTER.splitToList(querySegment);
if (keyValuePair.size() != 2) {
int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to ensure that there aren't more than 2 segments.

if (equalsIndex > 0) {
Copy link
Preview

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition equalsIndex > 0 excludes query parameters where the key is empty (e.g., =value). This changes behavior from the original code which allowed empty keys. Should be equalsIndex >= 0 to maintain compatibility.

Suggested change
if (equalsIndex > 0) {
if (equalsIndex >= 0) {

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition equalsIndex > 0 excludes valid query parameters that have empty keys (e.g., =value). Based on the test cases, this should be equalsIndex >= 0 to handle parameters with empty keys, or the logic should be restructured to explicitly handle the equalsIndex == 0 case.

Suggested change
if (equalsIndex > 0) {
if (equalsIndex >= 0) {

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition equalsIndex > 0 excludes valid query parameters that start with '=' (empty key). Based on the test cases, it appears '=' should be handled as a valid parameter with empty key and empty value. The condition should be equalsIndex >= 0 to handle this case.

Suggested change
if (equalsIndex > 0) {
if (equalsIndex >= 0) {

Copilot uses AI. Check for mistakes.

String key = querySegment.substring(0, equalsIndex);
String value = querySegment.substring(equalsIndex + 1);
url.queryParam(urlDecode(key), urlDecode(value));
} else {
Comment on lines +422 to +426
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd prefer if we wrote this as if-throw to avoid additional nesting in the non-exceptional case.

throw new SafeIllegalStateException(
"Expected two parameters",
SafeArg.of("parameters", keyValuePair.size()),
UnsafeArg.of("values", keyValuePair));
SafeArg.of("parameters", equalsIndex == -1 ? 0 : 1),
Copy link
Preview

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter count logic is incorrect. When equalsIndex == 0 (empty key), there are still 2 parameters (empty key + value), but this logic reports 1. The condition should distinguish between no equals sign (equalsIndex == -1) which is 0 parameters, empty key (equalsIndex == 0) which is 2 parameters, and the current > 0 case which is also 2 parameters.

Suggested change
SafeArg.of("parameters", equalsIndex == -1 ? 0 : 1),
SafeArg.of("parameters", equalsIndex == -1 ? 0 : 2),

Copilot uses AI. Check for mistakes.

UnsafeArg.of("values", querySegment));
}
Copy link
Preview

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation logic is incorrect. It checks for additional '=' characters starting from valueStart (after the first '='), but URLs can legitimately contain '=' characters in parameter values. This will incorrectly reject valid query parameters like 'param=value=with=equals'.

Suggested change
}
int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR);
if (equalsIndex > -1) {
String key = querySegment.substring(0, equalsIndex);
String value = querySegment.substring(equalsIndex + 1);
url.queryParam(urlDecode(key), urlDecode(value));
} else {
// No '=', treat as key with empty value
url.queryParam(urlDecode(querySegment), "");
}

Copilot uses AI. Check for mistakes.

url.queryParam(urlDecode(keyValuePair.get(0)), urlDecode(keyValuePair.get(1)));
}
}
}
Expand Down
63 changes: 63 additions & 0 deletions ...src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -95,6 +101,63 @@ 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("", "=", "value", "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=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=value&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", 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", 0), UnsafeArg.of("values", "query"));
verify(urlBuilder, never()).queryParam(any(), any());
}

@Test
public void testPostWithBody() {
Channel channel = stubNoContentResponseChannel();
Expand Down