diff --git a/spring-test/src/test/java/org/springframework/mock/http/server/reactive/MockServerHttpRequestTests.java b/spring-test/src/test/java/org/springframework/mock/http/server/reactive/MockServerHttpRequestTests.java index b2a4dff2ea2a..b2bac3ea887d 100644 --- a/spring-test/src/test/java/org/springframework/mock/http/server/reactive/MockServerHttpRequestTests.java +++ b/spring-test/src/test/java/org/springframework/mock/http/server/reactive/MockServerHttpRequestTests.java @@ -65,7 +65,7 @@ void queryParams() { .queryParam("name B", "value B1") .build(); - assertThat(request.getURI().toString()).isEqualTo("/foo%20bar?a=b&name%20A=value%20A1&name%20A=value%20A2&name%20B=value%20B1"); + assertThat(request.getURI().toString()).isEqualTo("/foo%20bar?a=b&name%20B=value%20B1&name%20A=value%20A1&name%20A=value%20A2"); } /** diff --git a/spring-web/src/main/java/org/springframework/web/util/HierarchicalUriComponents.java b/spring-web/src/main/java/org/springframework/web/util/HierarchicalUriComponents.java index 83a564638052..a6104184c9c7 100644 --- a/spring-web/src/main/java/org/springframework/web/util/HierarchicalUriComponents.java +++ b/spring-web/src/main/java/org/springframework/web/util/HierarchicalUriComponents.java @@ -25,16 +25,16 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.StringJoiner; import java.util.function.BiFunction; import java.util.function.UnaryOperator; +import java.util.stream.Collectors; import org.jspecify.annotations.Nullable; import org.springframework.util.Assert; -import org.springframework.util.CollectionUtils; -import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.ObjectUtils; import org.springframework.util.StreamUtils; @@ -50,6 +50,7 @@ * @author Sam Brannen * @since 3.1.3 * @see Hierarchical URIs + * */ @SuppressWarnings("serial") final class HierarchicalUriComponents extends UriComponents { @@ -58,8 +59,7 @@ final class HierarchicalUriComponents extends UriComponents { private static final String PATH_DELIMITER_STRING = String.valueOf(PATH_DELIMITER); - private static final MultiValueMap EMPTY_QUERY_PARAMS = - CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>()); + private static final List EMPTY_QUERY_PARAMS = List.of(); /** @@ -70,28 +70,35 @@ final class HierarchicalUriComponents extends UriComponents { public String getPath() { return ""; } + @Override public List getPathSegments() { return Collections.emptyList(); } + @Override public PathComponent encode(BiFunction encoder) { return this; } + @Override public void verify() { } + @Override public PathComponent expand(UriTemplateVariables uriVariables, @Nullable UnaryOperator encoder) { return this; } + @Override public void copyToUriComponentsBuilder(UriComponentsBuilder builder) { } + @Override public boolean equals(@Nullable Object other) { return (this == other); } + @Override public int hashCode() { return getClass().hashCode(); @@ -107,7 +114,7 @@ public int hashCode() { private final PathComponent path; - private final MultiValueMap queryParams; + private final List queryParams; private final EncodeState encodeState; @@ -116,18 +123,19 @@ public int hashCode() { /** * Package-private constructor. All arguments are optional, and can be {@code null}. - * @param scheme the scheme + * + * @param scheme the scheme * @param userInfo the user info - * @param host the host - * @param port the port - * @param path the path - * @param query the query parameters + * @param host the host + * @param port the port + * @param path the path + * @param query the query parameters * @param fragment the fragment - * @param encoded whether the components are already encoded + * @param encoded whether the components are already encoded */ HierarchicalUriComponents(@Nullable String scheme, @Nullable String fragment, @Nullable String userInfo, - @Nullable String host, @Nullable String port, @Nullable PathComponent path, - @Nullable MultiValueMap query, boolean encoded) { + @Nullable String host, @Nullable String port, @Nullable PathComponent path, + @Nullable List query, boolean encoded) { super(scheme, fragment); @@ -135,7 +143,7 @@ public int hashCode() { this.host = host; this.port = port; this.path = path != null ? path : NULL_PATH_COMPONENT; - this.queryParams = query != null ? CollectionUtils.unmodifiableMultiValueMap(query) : EMPTY_QUERY_PARAMS; + this.queryParams = query != null ? Collections.unmodifiableList(query) : EMPTY_QUERY_PARAMS; this.encodeState = encoded ? EncodeState.FULLY_ENCODED : EncodeState.RAW; // Check for illegal characters.. @@ -145,9 +153,9 @@ public int hashCode() { } private HierarchicalUriComponents(@Nullable String scheme, @Nullable String fragment, - @Nullable String userInfo, @Nullable String host, @Nullable String port, - PathComponent path, MultiValueMap queryParams, - EncodeState encodeState, @Nullable UnaryOperator variableEncoder) { + @Nullable String userInfo, @Nullable String host, @Nullable String port, + PathComponent path, List queryParams, + EncodeState encodeState, @Nullable UnaryOperator variableEncoder) { super(scheme, fragment); @@ -208,27 +216,9 @@ public List getPathSegments() { @Override public @Nullable String getQuery() { if (!this.queryParams.isEmpty()) { - StringBuilder queryBuilder = new StringBuilder(); - this.queryParams.forEach((name, values) -> { - if (CollectionUtils.isEmpty(values)) { - if (queryBuilder.length() != 0) { - queryBuilder.append('&'); - } - queryBuilder.append(name); - } - else { - for (Object value : values) { - if (queryBuilder.length() != 0) { - queryBuilder.append('&'); - } - queryBuilder.append(name); - if (value != null) { - queryBuilder.append('=').append(value.toString()); - } - } - } - }); - return queryBuilder.toString(); + return this.queryParams.stream() + .map(QueryParam::toUriString) + .collect(Collectors.joining("&")); } else { return null; @@ -240,7 +230,9 @@ public List getPathSegments() { */ @Override public MultiValueMap getQueryParams() { - return this.queryParams; + Map> collected = this.queryParams.stream().collect(Collectors.groupingBy(QueryParam::name, + Collectors.mapping(QueryParam::value, Collectors.toList()))); + return MultiValueMap.fromMultiValue(collected); } @@ -265,7 +257,7 @@ HierarchicalUriComponents encodeTemplate(Charset charset) { String userInfoTo = (getUserInfo() != null ? encoder.apply(getUserInfo(), Type.USER_INFO) : null); String hostTo = (getHost() != null ? encoder.apply(getHost(), getHostType()) : null); PathComponent pathTo = this.path.encode(encoder); - MultiValueMap queryParamsTo = encodeQueryParams(encoder); + List queryParamsTo = encodeQueryParams(encoder); return new HierarchicalUriComponents(schemeTo, fragmentTo, userInfoTo, hostTo, this.port, pathTo, queryParamsTo, EncodeState.TEMPLATE_ENCODED, this.variableEncoder); @@ -284,32 +276,22 @@ public HierarchicalUriComponents encode(Charset charset) { String hostTo = (this.host != null ? encodeUriComponent(this.host, charset, getHostType()) : null); BiFunction encoder = (s, type) -> encodeUriComponent(s, charset, type); PathComponent pathTo = this.path.encode(encoder); - MultiValueMap queryParamsTo = encodeQueryParams(encoder); + List queryParamsTo = encodeQueryParams(encoder); return new HierarchicalUriComponents(schemeTo, fragmentTo, userInfoTo, hostTo, this.port, pathTo, queryParamsTo, EncodeState.FULLY_ENCODED, null); } - private MultiValueMap encodeQueryParams(BiFunction encoder) { - int size = this.queryParams.size(); - MultiValueMap result = new LinkedMultiValueMap<>(size); - this.queryParams.forEach((key, values) -> { - String name = encoder.apply(key, Type.QUERY_PARAM); - List encodedValues = new ArrayList<>(values.size()); - for (String value : values) { - encodedValues.add(value != null ? encoder.apply(value, Type.QUERY_PARAM) : null); - } - result.put(name, encodedValues); - }); - return CollectionUtils.unmodifiableMultiValueMap(result); + private List encodeQueryParams(BiFunction encoder) { + return this.queryParams.stream().map(q -> q.encodeQueryParam(encoder)).toList(); } /** * Encode the given source into an encoded String using the rules specified * by the given component and with the given options. - * @param source the source String + * @param source the source String * @param encoding the encoding of the source String - * @param type the URI component for the source + * @param type the URI component for the source * @return the encoded URI * @throws IllegalArgumentException when the given value is not a valid URI component */ @@ -320,9 +302,9 @@ static String encodeUriComponent(String source, String encoding, Type type) { /** * Encode the given source into an encoded String using the rules specified * by the given component and with the given options. - * @param source the source String + * @param source the source String * @param charset the encoding of the source String - * @param type the URI component for the source + * @param type the URI component for the source * @return the encoded URI * @throws IllegalArgumentException when the given value is not a valid URI component */ @@ -376,12 +358,7 @@ private void verify() { verifyUriComponent(this.userInfo, Type.USER_INFO); verifyUriComponent(this.host, getHostType()); this.path.verify(); - this.queryParams.forEach((key, values) -> { - verifyUriComponent(key, Type.QUERY_PARAM); - for (String value : values) { - verifyUriComponent(value, Type.QUERY_PARAM); - } - }); + this.queryParams.forEach(QueryParam::verifyUriComponent); verifyUriComponent(getFragment(), Type.FRAGMENT); } @@ -430,25 +407,18 @@ protected HierarchicalUriComponents expandInternal(UriTemplateVariables uriVaria String hostTo = expandUriComponent(this.host, uriVariables, this.variableEncoder); String portTo = expandUriComponent(this.port, uriVariables, this.variableEncoder); PathComponent pathTo = this.path.expand(uriVariables, this.variableEncoder); - MultiValueMap queryParamsTo = expandQueryParams(uriVariables); + List queryParamsTo = expandQueryParams(uriVariables); String fragmentTo = expandUriComponent(getFragment(), uriVariables, this.variableEncoder); return new HierarchicalUriComponents(schemeTo, fragmentTo, userInfoTo, hostTo, portTo, pathTo, queryParamsTo, this.encodeState, this.variableEncoder); } - private MultiValueMap expandQueryParams(UriTemplateVariables variables) { - int size = this.queryParams.size(); - MultiValueMap result = new LinkedMultiValueMap<>(size); + private List expandQueryParams(UriTemplateVariables variables) { UriTemplateVariables queryVariables = new QueryUriTemplateVariables(variables); - this.queryParams.forEach((key, values) -> { - String name = expandUriComponent(key, queryVariables, this.variableEncoder); - List expandedValues = result.computeIfAbsent(name, k -> new ArrayList<>(values.size())); - for (String value : values) { - expandedValues.add(expandUriComponent(value, queryVariables, this.variableEncoder)); - } - }); - return CollectionUtils.unmodifiableMultiValueMap(result); + return this.queryParams.stream() + .map(qp -> qp.expandUriComponent(queryVariables, this.variableEncoder)) + .toList(); } @Override @@ -568,6 +538,7 @@ public int hashCode() { /** * Enumeration used to identify the allowed characters per URI component. *

Contains methods to indicate whether a given character is valid in a specific URI component. + * * @see RFC 3986 */ enum Type { @@ -749,7 +720,7 @@ private enum EncodeState { * URI template encoded first by quoting illegal characters only, and * then URI vars encoded more strictly when expanded, by quoting both * illegal chars and chars with reserved meaning. - */ + */ TEMPLATE_ENCODED; @@ -833,7 +804,7 @@ else if (level > 0) { * for example, {@code "/{year:\d{1,4}}"}. */ private boolean isUriVariable(CharSequence source) { - if (source.length() < 2 || source.charAt(0) != '{' || source.charAt(source.length() -1) != '}') { + if (source.length() < 2 || source.charAt(0) != '{' || source.charAt(source.length() - 1) != '}') { return false; } boolean hasText = false; @@ -1097,4 +1068,29 @@ else if (value instanceof Collection collection) { } } + record QueryParam(String name, @Nullable String value) implements Serializable{ + public String toUriString() { + return this.name + (this.value == null ? "" : "=" + this.value); + } + + public QueryParam encodeQueryParam(BiFunction encoder) { + return new QueryParam(encoder.apply(this.name, Type.QUERY_PARAM), + this.value != null ? encoder.apply(this.value, Type.QUERY_PARAM) : null); + } + + public void verifyUriComponent() { + HierarchicalUriComponents.verifyUriComponent(this.name, Type.QUERY_PARAM); + HierarchicalUriComponents.verifyUriComponent(this.value, Type.QUERY_PARAM); + } + + public QueryParam expandUriComponent(UriTemplateVariables queryVariables, @Nullable UnaryOperator variableEncoder) { + return new QueryParam(Objects.requireNonNull(UriComponents.expandUriComponent(this.name, queryVariables, variableEncoder)), + UriComponents.expandUriComponent(this.value, queryVariables, variableEncoder)); + } + + public boolean hasName(String name) { + return name.equals(this.name); + } + } + } diff --git a/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java b/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java index e8783e4cf2e0..3e1cf162d0cf 100644 --- a/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java +++ b/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java @@ -34,7 +34,6 @@ import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; -import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; @@ -89,7 +88,7 @@ public class UriComponentsBuilder implements UriBuilder, Cloneable { private CompositePathComponentBuilder pathBuilder; - private final MultiValueMap queryParams = new LinkedMultiValueMap<>(); + private final List queryParams = new ArrayList<>(); private @Nullable String fragment; @@ -286,7 +285,7 @@ private UriComponents buildInternal(EncodingHint hint) { result = new OpaqueUriComponents(this.scheme, this.ssp, this.fragment); } else { - MultiValueMap queryParams = new LinkedMultiValueMap<>(this.queryParams); + List queryParams = new ArrayList<>(this.queryParams); HierarchicalUriComponents uric = new HierarchicalUriComponents(this.scheme, this.fragment, this.userInfo, this.host, this.port, this.pathBuilder.build(), queryParams, hint == EncodingHint.FULLY_ENCODED); @@ -575,11 +574,11 @@ public UriComponentsBuilder queryParam(String name, @Nullable Object... values) if (!ObjectUtils.isEmpty(values)) { for (Object value : values) { String valueAsString = getQueryParamValue(value); - this.queryParams.add(name, valueAsString); + this.queryParams.add(new HierarchicalUriComponents.QueryParam(name, valueAsString)); } } else { - this.queryParams.add(name, null); + this.queryParams.add(new HierarchicalUriComponents.QueryParam(name, null)); } resetSchemeSpecificPart(); return this; @@ -619,16 +618,27 @@ public UriComponentsBuilder queryParamIfPresent(String name, Optional value) @Override public UriComponentsBuilder queryParams(@Nullable MultiValueMap params) { if (params != null) { - this.queryParams.addAll(params); + addAllToQueryParams(params); resetSchemeSpecificPart(); } return this; } + private void addAllToQueryParams(MultiValueMap params) { + params.forEach((key, values) -> { + if (values.isEmpty()) { + this.queryParams.add(new HierarchicalUriComponents.QueryParam(key, null)); + } + else{ + values.forEach(v -> this.queryParams.add(new HierarchicalUriComponents.QueryParam(key, v))); + } + }); + } + @Override public UriComponentsBuilder replaceQueryParam(String name, Object... values) { Assert.notNull(name, "Name must not be null"); - this.queryParams.remove(name); + this.queryParams.removeIf(qp->qp.hasName(name)); if (!ObjectUtils.isEmpty(values)) { queryParam(name, values); } @@ -649,7 +659,7 @@ public UriComponentsBuilder replaceQueryParam(String name, @Nullable Collection< public UriComponentsBuilder replaceQueryParams(@Nullable MultiValueMap params) { this.queryParams.clear(); if (params != null) { - this.queryParams.putAll(params); + addAllToQueryParams(params); } return this; } diff --git a/spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java index 349834760492..6e6de9197283 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java @@ -114,8 +114,9 @@ void queryStringIncluded() throws Exception { applyFilter(); - assertThat(filter.beforeRequestMessage).contains("/hotels?booking=42&code=masked&category=hotel&category=resort&ignore=masked"); - assertThat(filter.afterRequestMessage).contains("/hotels?booking=42&code=masked&category=hotel&category=resort&ignore=masked"); + String expectedRequest = "/hotels?booking=42&code=masked&ignore=masked&category=hotel&category=resort"; + assertThat(filter.beforeRequestMessage).contains(expectedRequest); + assertThat(filter.afterRequestMessage).contains(expectedRequest); } @Test diff --git a/spring-web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java b/spring-web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java index 991b91801d7f..94e6a46ac568 100644 --- a/spring-web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java @@ -17,6 +17,7 @@ package org.springframework.web.util; import java.net.URI; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -25,9 +26,11 @@ import java.util.Map; import java.util.Optional; import java.util.function.BiConsumer; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.EnumSource; import org.springframework.util.LinkedMultiValueMap; @@ -930,4 +933,50 @@ void expandPortAndPathWithoutSeparator(ParserType parserType) { assertThat(uri.toString()).isEqualTo("ws://localhost:7777/test"); } + @ParameterizedTest(name = "{0} for {3}, {4}, {5} with parser {1}") //gh 34788 + @CsvSource(textBlock = """ + #testedPattern parserType urlExpected p1 p2 p3 + 'http://myhost?a={p1}&b={p2}&a={p3}',RFC, 'http://myhost?a=a1&b=b1&a=a2','a1','b1','a2' + 'http://myhost?a={p1}&b={p2}&a={p3}',WHAT_WG, 'http://myhost?a=a1&b=b1&a=a2','a1','b1','a2' + 'http://myhost?a={p1}&b={p2}&a={p1}',RFC, 'http://myhost?a=a1&b=b1&a=a1','a1','b1','a1' + 'http://myhost?a={p1}&a={p1}&b={p3}',RFC, 'http://myhost?a=a1&a=a1&b=b1','a1','a1','b1', + 'http://myhost?a={p1}&a={p2}&b={p3}',RFC, 'http://myhost?a=a1&a=a2&b=b1','a1','a2','b1' + 'http://myhost?a={p1}&b={p2}&a={p1}',RFC, 'http://myhost?a=a1&b=&a=a1','a1','','a1' + 'http://myhost?a={p1}&b=&a={p2}', RFC, 'http://myhost?a=a1&b=&a=a2','a1','a2', + 'http://myhost?a={p1}&b=t&a={p2}', RFC, 'http://myhost?a=a1&b=t&a=a2','a1','a2', + 'http://myhost?a=t&b={p1}&a={p2}', RFC, 'http://myhost?a=t&b=b1&a=a1','b1','a1', + 'http://myhost?a={p1}&b={p2}&a={p1}',WHAT_WG, 'http://myhost?a=a1&b=b1&a=a1','a1','b1','a1' + 'http://myhost?a={p1}&a={p1}&b={p3}',WHAT_WG, 'http://myhost?a=a1&a=a1&b=b1','a1','a1','b1', + 'http://myhost?a={p1}&a={p2}&b={p3}',WHAT_WG, 'http://myhost?a=a1&a=a2&b=b1','a1','a2','b1' + 'http://myhost?a={p1}&b={p2}&a={p1}',WHAT_WG, 'http://myhost?a=a1&b=&a=a1','a1','','a1' + 'http://myhost?a={p1}&b=&a={p2}', WHAT_WG, 'http://myhost?a=a1&b=&a=a2','a1','a2', + 'http://myhost?a={p1}&b=t&a={p2}', WHAT_WG, 'http://myhost?a=a1&b=t&a=a2','a1','a2', + 'http://myhost?a=t&b={p1}&a={p2}', WHAT_WG, 'http://myhost?a=t&b=b1&a=a1','b1','a1', + """) + void queryParamOrderShouldBeKept(String testedPattern, ParserType parserType, String urlexpected, String p1, String p2, String p3){ + ArrayList params = new ArrayList<>(); + Stream.of(p1, p2, p3).forEach(p->addIfNotNull(p, params)); + Map paramsMap = new HashMap<>(); + putIfNotNull("p1", p1, paramsMap); + putIfNotNull("p2", p2, paramsMap); + putIfNotNull("p3", p3, paramsMap); + UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(testedPattern, parserType); + assertThat(uriComponentsBuilder).satisfies( + ucb -> assertThat(ucb.buildAndExpand(params.toArray())).as("with params as varags").hasToString(urlexpected), + ucb -> assertThat(ucb.buildAndExpand(paramsMap)).as("with params as Map").hasToString(urlexpected) + ); + } + + private static void putIfNotNull(String key, String param, Map paramsMap) { + if (param != null) { + paramsMap.put(key, param); + } + } + + private static void addIfNotNull(String param, ArrayList params) { + if (param !=null){ + params.add(param); + } + } + }