diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientForm.java b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientForm.java index ed28ef7c70..25aadfcc6e 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientForm.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientForm.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017-2025 VMware, Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2017-2026 VMware, Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -49,7 +49,7 @@ public interface HttpClientForm { /** * Should file attributes be cleaned and eventually removed from disk. - * Default to false. + * Default to true. * * @param clean true if cleaned on termination (successful or failed) * diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFormEncoder.java b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFormEncoder.java index 7804c718bc..b8847d7d3f 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFormEncoder.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientFormEncoder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011-2025 VMware, Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2011-2026 VMware, Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -53,6 +53,7 @@ final class HttpClientFormEncoder extends HttpPostRequestEncoder HttpDataFactory newFactory; boolean cleanOnTerminate; Charset newCharset; + @Nullable HttpClientFormEncoder nextEncoder; boolean newMultipart; EncoderMode newMode; @@ -101,6 +102,10 @@ public HttpContent readChunk(ByteBufAllocator allocator) throws Exception { @Override public HttpClientForm attr(String name, String value) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.attr(name, value); + } try { addBodyAttribute(name, value); } @@ -112,6 +117,10 @@ public HttpClientForm attr(String name, String value) { @Override public HttpClientForm charset(Charset charset) { + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + return encoder.charset(charset); + } this.newCharset = Objects.requireNonNull(charset, "charset"); this.needNewEncoder = true; return this; @@ -119,12 +128,20 @@ public HttpClientForm charset(Charset charset) { @Override public HttpClientForm cleanOnTerminate(boolean clean) { + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + return encoder.cleanOnTerminate(clean); + } this.cleanOnTerminate = clean; return this; } @Override public HttpClientForm factory(HttpDataFactory factory) { + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + return encoder.factory(factory); + } if (!getBodyListAttributes().isEmpty()) { throw new IllegalStateException("Cannot set a new HttpDataFactory after " + "starting appending Parts, call factory(f) at the earliest occasion" + @@ -152,6 +169,10 @@ public HttpClientForm file(String name, String filename, File file, @Nullable String contentType) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.file(name, filename, file, contentType); + } Objects.requireNonNull(name, "name"); Objects.requireNonNull(file, "file"); Objects.requireNonNull(filename, "filename"); @@ -184,6 +205,10 @@ public HttpClientForm file(String name, String filename, InputStream stream, @Nullable String contentType) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.file(name, filename, stream, contentType); + } Objects.requireNonNull(name, "name"); Objects.requireNonNull(stream, "stream"); try { @@ -226,6 +251,10 @@ public HttpClientForm files(String name, File[] files, String[] contentTypes, boolean[] textFiles) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.files(name, files, contentTypes, textFiles); + } try { addBodyFileUploads(name, files, contentTypes, textFiles); } @@ -237,6 +266,10 @@ public HttpClientForm files(String name, @Override public HttpClientForm encoding(EncoderMode mode) { + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + return encoder.encoding(mode); + } this.newMode = Objects.requireNonNull(mode, "mode"); this.needNewEncoder = true; return this; @@ -244,7 +277,11 @@ public HttpClientForm encoding(EncoderMode mode) { @Override public HttpClientForm multipart(boolean isMultipart) { - this.needNewEncoder = isChunked() != isMultipart; + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + return encoder.multipart(isMultipart); + } + this.needNewEncoder |= isMultipart() != isMultipart; this.newMultipart = isMultipart; return this; } @@ -257,6 +294,10 @@ public HttpClientForm textFile(String name, File file) { @Override public HttpClientForm textFile(String name, File file, @Nullable String contentType) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.textFile(name, file, contentType); + } try { addBodyFileUpload(name, file, contentType, true); } @@ -276,6 +317,10 @@ public HttpClientForm textFile(String name, InputStream stream) { public HttpClientForm textFile(String name, InputStream stream, @Nullable String contentType) { + HttpClientFormEncoder encoder = applyChanges(request); + if (encoder != this) { + return encoder.textFile(name, stream, contentType); + } Objects.requireNonNull(name, "name"); Objects.requireNonNull(stream, "stream"); try { @@ -310,7 +355,21 @@ public void run() { cleanFiles(); } + @Override + public void cleanFiles() { + HttpClientFormEncoder encoder = currentEncoder(); + if (encoder != this) { + encoder.cleanFiles(); + } + else { + super.cleanFiles(); + } + } + final HttpClientFormEncoder applyChanges(HttpRequest request) { + if (nextEncoder != null) { + return nextEncoder.applyChanges(request); + } if (!needNewEncoder) { return this; } @@ -322,6 +381,8 @@ final HttpClientFormEncoder applyChanges(HttpRequest request) { newCharset, newMode); + encoder.cleanOnTerminate = cleanOnTerminate; + nextEncoder = encoder; encoder.setBodyHttpDatas(getBodyListAttributes()); needNewEncoder = false; @@ -333,6 +394,14 @@ final HttpClientFormEncoder applyChanges(HttpRequest request) { } } + private HttpClientFormEncoder currentEncoder() { + HttpClientFormEncoder encoder = this; + while (encoder.nextEncoder != null) { + encoder = encoder.nextEncoder; + } + return encoder; + } + static final Map percentEncodings = new HashMap<>(); static final String DEFAULT_BINARY_CONTENT_TYPE = "application/octet-stream"; diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java index e137ded97f..166e38d33d 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java @@ -1154,9 +1154,10 @@ public void subscribe(CoreSubscriber s) { @SuppressWarnings("FutureReturnValueIgnored") void _subscribe(CoreSubscriber s) { HttpDataFactory df = DEFAULT_FACTORY; + HttpClientFormEncoder encoder = null; try { - HttpClientFormEncoder encoder = new HttpClientFormEncoder(df, + encoder = new HttpClientFormEncoder(df, parent.nettyRequest, false, HttpConstants.DEFAULT_CHARSET, @@ -1227,7 +1228,12 @@ void _subscribe(CoreSubscriber s) { } catch (Throwable e) { Exceptions.throwIfJvmFatal(e); - df.cleanRequestHttpData(parent.nettyRequest); + if (encoder != null) { + encoder.cleanFiles(); + } + else { + df.cleanRequestHttpData(parent.nettyRequest); + } s.onError(Exceptions.unwrap(e)); } } diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientFormEncoderTests.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientFormEncoderTests.java new file mode 100644 index 0000000000..65218933a1 --- /dev/null +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientFormEncoderTests.java @@ -0,0 +1,199 @@ +/* + * Copyright (c) 2026 VMware, Inc. or its affiliates, All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package reactor.netty.http.client; + +import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.codec.http.multipart.Attribute; +import io.netty.handler.codec.http.multipart.DefaultHttpDataFactory; +import io.netty.handler.codec.http.multipart.HttpDataFactory; +import io.netty.handler.codec.http.multipart.HttpPostRequestEncoder; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.nio.charset.StandardCharsets; +import java.util.function.Consumer; + +import static org.assertj.core.api.Assertions.assertThat; + +class HttpClientFormEncoderTests { + + @Test + void charsetIsPreservedWhenMultipartIsConfiguredLast() throws Exception { + String body = encode(form -> form.charset(StandardCharsets.ISO_8859_1) + .multipart(false), "é"); + + assertThat(body).isEqualTo("key=%E9"); + } + + @Test + void encodingIsPreservedWhenMultipartIsConfiguredLast() throws Exception { + String body = encode(form -> form.encoding(HttpPostRequestEncoder.EncoderMode.RFC3986) + .multipart(false), "*"); + + assertThat(body).isEqualTo("key=%2A"); + } + + @Test + void subsequentCallsUseTheLatestEncoder() throws Exception { + DefaultFullHttpRequest request = newRequest(); + HttpClientFormEncoder encoder = newEncoder(new DefaultHttpDataFactory(false), request); + + try { + encoder.charset(StandardCharsets.ISO_8859_1); + encoder.attr("first", "é"); + encoder.encoding(HttpPostRequestEncoder.EncoderMode.RFC3986); + encoder.multipart(false); + encoder.attr("second", "*"); + + encoder = encoder.applyChanges(request); + encoder.finalizeRequest(); + + assertThat(request.content().toString(StandardCharsets.US_ASCII)) + .isEqualTo("first=%E9&second=%2A"); + } + finally { + encoder.cleanFiles(); + request.release(); + } + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void customFactoryIsUsedRegardlessOfMultipartSetterOrder(boolean factoryFirst) throws Exception { + CountingHttpDataFactory defaultFactory = new CountingHttpDataFactory(); + CountingHttpDataFactory customFactory = new CountingHttpDataFactory(); + DefaultFullHttpRequest request = newRequest(); + HttpClientFormEncoder encoder = newEncoder(defaultFactory, request); + + try { + if (factoryFirst) { + encoder.factory(customFactory).multipart(false); + } + else { + encoder.multipart(false).factory(customFactory); + } + encoder.attr("key", "value"); + + encoder = encoder.applyChanges(request); + encoder.finalizeRequest(); + encoder.cleanFiles(); + + assertThat(defaultFactory.createAttributeCalls).isZero(); + assertThat(defaultFactory.cleanRequestCalls).isZero(); + assertThat(customFactory.createAttributeCalls).isEqualTo(2); + assertThat(customFactory.cleanRequestCalls).isOne(); + } + finally { + request.release(); + } + } + + @Test + void cleanOnTerminateIsPreservedWhenCharsetChanges() throws Exception { + assertCleanOnTerminateIsPreserved(form -> form.charset(StandardCharsets.ISO_8859_1)); + } + + @Test + void cleanOnTerminateIsPreservedWhenEncodingChanges() throws Exception { + assertCleanOnTerminateIsPreserved( + form -> form.encoding(HttpPostRequestEncoder.EncoderMode.RFC3986)); + } + + @Test + void cleanOnTerminateIsPreservedWhenMultipartChanges() throws Exception { + assertCleanOnTerminateIsPreserved(form -> form.multipart(true)); + } + + @Test + void cleanOnTerminateIsPreservedWhenFactoryChanges() throws Exception { + assertCleanOnTerminateIsPreserved(form -> form.factory(new DefaultHttpDataFactory(false))); + } + + private static void assertCleanOnTerminateIsPreserved(Consumer formChange) throws Exception { + DefaultFullHttpRequest request = newRequest(); + HttpClientFormEncoder encoder = newEncoder(new DefaultHttpDataFactory(false), request); + + try { + encoder.cleanOnTerminate(false); + formChange.accept(encoder); + + HttpClientFormEncoder changedEncoder = encoder.applyChanges(request); + + assertThat(changedEncoder).isNotSameAs(encoder); + assertThat(changedEncoder.cleanOnTerminate).isFalse(); + } + finally { + request.release(); + } + } + + private static String encode(Consumer formConfig, String value) throws Exception { + DefaultFullHttpRequest request = newRequest(); + HttpClientFormEncoder encoder = newEncoder(new DefaultHttpDataFactory(false), request); + + try { + formConfig.accept(encoder); + encoder.attr("key", value); + + encoder = encoder.applyChanges(request); + encoder.finalizeRequest(); + + return request.content().toString(StandardCharsets.US_ASCII); + } + finally { + encoder.cleanFiles(); + request.release(); + } + } + + private static HttpClientFormEncoder newEncoder(HttpDataFactory factory, HttpRequest request) throws Exception { + return new HttpClientFormEncoder(factory, + request, + false, + StandardCharsets.UTF_8, + HttpPostRequestEncoder.EncoderMode.RFC1738); + } + + private static DefaultFullHttpRequest newRequest() { + return new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/"); + } + + static final class CountingHttpDataFactory extends DefaultHttpDataFactory { + + int cleanRequestCalls; + int createAttributeCalls; + + CountingHttpDataFactory() { + super(false); + } + + @Override + public Attribute createAttribute(HttpRequest request, String name, String value) { + createAttributeCalls++; + return super.createAttribute(request, name, value); + } + + @Override + public void cleanRequestHttpData(HttpRequest request) { + cleanRequestCalls++; + super.cleanRequestHttpData(request); + } + } +} diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientWithTomcatTest.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientWithTomcatTest.java index 862d198130..42e0c90be3 100644 --- a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientWithTomcatTest.java +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientWithTomcatTest.java @@ -27,6 +27,8 @@ import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.multipart.DefaultHttpDataFactory; import io.netty.handler.codec.http.multipart.HttpData; +import io.netty.handler.codec.http.multipart.HttpDataFactory; +import io.netty.handler.codec.http.multipart.HttpPostRequestEncoder; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -43,6 +45,7 @@ import java.lang.reflect.Field; import java.net.SocketAddress; import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -124,15 +127,55 @@ void postUploadNoMultipart() throws Exception { @Test void postUploadNoMultipartWithCustomFactory() throws Exception { + DefaultHttpDataFactory customFactory = new DefaultHttpDataFactory(DefaultHttpDataFactory.MINSIZE); doTestPostUpload((req, form) -> { - DefaultHttpDataFactory customFactory = new DefaultHttpDataFactory(DefaultHttpDataFactory.MINSIZE); form.factory(customFactory) .multipart(false) .attr("attr1", "attr2"); }, "attr1=attr2"); + assertThat(getRequestFileDeleteMap(customFactory)).isEmpty(); + } + + @Test + void customFactoryIsCleanedWhenFormCallbackFails() throws Exception { + DefaultHttpDataFactory customFactory = new DefaultHttpDataFactory(DefaultHttpDataFactory.MINSIZE); + HttpClient client = HttpClient.create() + .host("localhost") + .port(getPort()); + + StepVerifier.create( + client.post() + .uri("/multipart") + .sendForm((req, form) -> { + form.factory(customFactory) + .multipart(false) + .attr("attr1", "attr2"); + throw new IllegalStateException("test"); + }) + .response()) + .expectErrorMessage("test") + .verify(Duration.ofSeconds(30)); + + assertThat(getRequestFileDeleteMap(DEFAULT_FACTORY)).isEmpty(); + assertThat(getRequestFileDeleteMap(customFactory)).isEmpty(); + } + + @Test + void postUploadNoMultipartWithCustomCharset() throws Exception { + doTestPostUpload((req, form) -> form.charset(StandardCharsets.ISO_8859_1) + .multipart(false) + .attr("attr1", "é"), + "attr1=%E9"); + } + + @Test + void postUploadNoMultipartWithRfc3986Encoding() throws Exception { + doTestPostUpload((req, form) -> form.encoding(HttpPostRequestEncoder.EncoderMode.RFC3986) + .multipart(false) + .attr("attr1", "*"), + "attr1=%2A"); } - @SuppressWarnings("unchecked") private static void doTestPostUpload(BiConsumer formCallback, String expectedResponse) throws Exception { HttpClient client = @@ -152,9 +195,15 @@ private static void doTestPostUpload(BiConsumer> getRequestFileDeleteMap(HttpDataFactory factory) + throws Exception { + Field field = DefaultHttpDataFactory.class.getDeclaredField("requestFileDeleteMap"); field.setAccessible(true); - assertThat((Map>) field.get(DEFAULT_FACTORY)).isEmpty(); + return (Map>) field.get(factory); } @Test