Skip to content

replacing StringBuilder with StringJoiner in order to simplify logic #4628

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
renatoh wants to merge 6 commits into
base: main
Choose a base branch
from
+73 −37
Open

replacing StringBuilder with StringJoiner in order to simplify logic #4628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22ca3fa
replacing StringBuilder with StringJoiner in order to simplify logic …
renatoh Jul 9, 2026
6e73a65
remove unnecessary set of emptyValue on StringJoiner
renatoh Jul 9, 2026
b7d7a02
remove unnecessary set of emptyValue on StringJoiner
renatoh Jul 9, 2026
2c2c965
Update solr/solrj/src/test/org/apache/solr/common/params/SolrParamTes…
renatoh Jul 23, 2026
0abc66c
Url-encode name only once
renatoh Jul 23, 2026
e244aff
fix formatting
renatoh Jul 23, 2026
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
11 changes: 5 additions & 6 deletions solr/core/src/java/org/apache/solr/jersey/PostRequestLoggingFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.StringJoiner;
import java.util.stream.Collectors;
import org.apache.solr.client.api.model.SolrJerseyResponse;
import org.apache.solr.common.util.CollectionUtil;
Expand Down Expand Up @@ -164,7 +165,7 @@ public static String buildRequestBodyString(ContainerRequestContext requestConte
public static String filterAndStringifyQueryParameters(
MultivaluedMap<String, String> unfilteredParams) {
final var paramNamesToLog = getParamNamesToLog(unfilteredParams);
final StringBuilder sb = new StringBuilder(128);
var output = new StringJoiner("&");
unfilteredParams.entrySet().stream()
.sorted(Map.Entry.comparingByKey())
.forEachOrdered(
Expand All @@ -173,13 +174,11 @@ public static String filterAndStringifyQueryParameters(
if (!paramNamesToLog.contains(name)) return;

for (String val : entry.getValue()) {
if (sb.length() != 0) sb.append('&');
StrUtils.partialURLEncodeVal(sb, name);
sb.append('=');
StrUtils.partialURLEncodeVal(sb, val);
output.add(
StrUtils.partialURLEncodeVal(name) + "=" + StrUtils.partialURLEncodeVal(val));
}
});
return sb.toString();
return output.toString();
}

private static Set<String> getParamNamesToLog(MultivaluedMap<String, String> queryParameters) {
Expand Down
28 changes: 11 additions & 17 deletions solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.StringJoiner;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import org.apache.solr.client.solrj.util.ClientUtils;
Expand Down Expand Up @@ -459,19 +460,17 @@ public NamedList<Object> toNamedList() {
*/
public String toQueryString() {
final Charset charset = StandardCharsets.UTF_8;
final StringBuilder sb = new StringBuilder(128);
boolean first = true;

var output = new StringJoiner("&", "?", "");

@epugh epugh Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

interesting pattern, I've never seen StringJoiner before.

@renatoh renatoh Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @epugh, it has been around for a while, since Java 8, but I have only discovered it recently and since then I have been using it quite frequently, it saves you exactly those isFirst checks.

output.setEmptyValue("");
for (final Iterator<String> it = getParameterNamesIterator(); it.hasNext(); ) {
final String name = it.next(), nameEnc = URLEncoder.encode(name, charset);
final String name = it.next();
final String nameEnc = URLEncoder.encode(name, charset);
for (String val : getParams(name)) {
sb.append(first ? '?' : '&')
.append(nameEnc)
.append('=')
.append(URLEncoder.encode(val, charset));
first = false;
output.add(nameEnc + "=" + URLEncoder.encode(val, charset));
}
}
return sb.toString();
return output.toString();
}

/**
Expand Down Expand Up @@ -510,19 +509,14 @@ public String toLocalParamsString() {
*/
@Override
public String toString() {
final StringBuilder sb = new StringBuilder(128);
boolean first = true;
StringJoiner query = new StringJoiner("&");
for (final Iterator<String> it = getParameterNamesIterator(); it.hasNext(); ) {
final String name = it.next();
for (String val : getParams(name)) {
if (!first) sb.append('&');
first = false;
StrUtils.partialURLEncodeVal(sb, name);
sb.append('=');
StrUtils.partialURLEncodeVal(sb, val);
query.add(StrUtils.partialURLEncodeVal(name) + "=" + StrUtils.partialURLEncodeVal(val));
}
}
return sb.toString();
return query.toString();
}

/**
Expand Down
22 changes: 12 additions & 10 deletions solr/solrj/src/java/org/apache/solr/common/util/StrUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,36 +302,38 @@ public static boolean parseBool(String s, boolean def) {
*
* <p>Characters with a numeric value less than 32 are encoded. &amp;,=,%,+,space are encoded.
*/
public static void partialURLEncodeVal(StringBuilder dest, String val) {
public static String partialURLEncodeVal(String val) {
var output = new StringBuilder();
for (int i = 0; i < val.length(); i++) {
char ch = val.charAt(i);
if (ch < 32) {
dest.append('%');
if (ch < 0x10) dest.append('0');
dest.append(Integer.toHexString(ch));
output.append('%');
if (ch < 0x10) output.append('0');
output.append(Integer.toHexString(ch));
} else {
switch (ch) {
case ' ':
dest.append('+');
output.append('+');
break;
case '&':
dest.append("%26");
output.append("%26");
break;
case '%':
dest.append("%25");
output.append("%25");
break;
case '=':
dest.append("%3D");
output.append("%3D");
break;
case '+':
dest.append("%2B");
output.append("%2B");
break;
default:
dest.append(ch);
output.append(ch);
break;
}
}
}
return output.toString();
}

/**
Expand Down
41 changes: 41 additions & 0 deletions solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import org.apache.solr.SolrTestCase;
import org.apache.solr.common.SolrException;
import org.apache.solr.search.QueryParsing;
import org.apache.solr.servlet.SolrRequestParsers;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -352,6 +354,45 @@ public void testGetParams() {
assertNull(defaults.get("asagdsaga"));
}

@Test
public void testFirstParamHasQuestionMark() {

final ModifiableSolrParams in = getDummySolrParams();
String queryString = in.toQueryString();
assertEquals('?', queryString.charAt(0));

MultiMapSolrParams out = SolrRequestParsers.parseQueryString(queryString.substring(1));
assertEquals(in, out);
}

@Test
public void testIfToStringCanBeParsed() {

final ModifiableSolrParams in = getDummySolrParams();
String queryString = in.toString();

assertEquals("first=1st&second=2nd&third=3rd", queryString);
MultiMapSolrParams out = SolrRequestParsers.parseQueryString(queryString);
assertEquals(in, out);
}

Comment thread
renatoh marked this conversation as resolved.
@Test
public void testToStringWithEmptySolrParams() {

ModifiableSolrParams in = new ModifiableSolrParams();
String queryString = in.toString();
assertEquals("", queryString);
MultiMapSolrParams out = SolrRequestParsers.parseQueryString(queryString);
assertEquals(in, out);
}

private ModifiableSolrParams getDummySolrParams() {
return params(
"first", "1st",
"second", "2nd",
"third", "3rd");
}

public static int getReturnCode(Runnable runnable) {
try {
runnable.run();
Expand Down
8 changes: 4 additions & 4 deletions solr/test-framework/src/java/org/apache/solr/util/RestTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ private static String setParam(String query, String paramToSet, String valueToSe
// empty query -> return "paramToSet=valueToSet"
builder.append(paramToSet);
builder.append('=');
StrUtils.partialURLEncodeVal(builder, valueToSet);
builder.append(StrUtils.partialURLEncodeVal(valueToSet));
return builder.toString();
}
MultiMapSolrParams requestParams = SolrRequestParsers.parseQueryString(query);
Expand All @@ -470,7 +470,7 @@ private static String setParam(String query, String paramToSet, String valueToSe
builder.append('&');
builder.append(paramToSet);
builder.append('=');
StrUtils.partialURLEncodeVal(builder, valueToSet);
builder.append(StrUtils.partialURLEncodeVal(valueToSet));
return builder.toString();
}
if (1 == values.length && valueToSet.equals(values[0])) {
Expand All @@ -490,14 +490,14 @@ private static String setParam(String query, String paramToSet, String valueToSe
isFirst = false;
builder.append(key);
builder.append('=');
StrUtils.partialURLEncodeVal(builder, null == val ? "" : val);
builder.append(StrUtils.partialURLEncodeVal(null == val ? "" : val));
}
}
}
builder.append(isFirst ? "" : '&');
builder.append(paramToSet);
builder.append('=');
StrUtils.partialURLEncodeVal(builder, valueToSet);
builder.append(StrUtils.partialURLEncodeVal(valueToSet));
return builder.toString();
}
}
Loading